Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify ssh box to fix multi-line command issues and add unit tests #1432

Closed
wants to merge 13 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 13 additions & 4 deletions .github/workflows/run-unit-tests.yml
Expand Up @@ -30,8 +30,13 @@ jobs:
version: latest
- name: Build Environment
run: make build
- name: Run Tests
run: poetry run pytest ./tests/unit
# Sandbox requires to import modules in fixture to change os.environ
# imports in other test files will cause issues, so we need to test sandbox separately
- name: Run Tests Except Sandbox
run: poetry run pytest ./tests/unit --ignore ./tests/unit/test_sandbox.py
- name: Run Tests For Sandbox
run: poetry run pytest ./tests/unit/test_sandbox.py

test-on-linux:
name: Test on Linux
runs-on: ubuntu-latest
Expand All @@ -49,8 +54,12 @@ jobs:
run: curl -sSL https://install.python-poetry.org | python3 -
- name: Build Environment
run: make build
- name: Run Tests
run: poetry run pytest ./tests/unit
# Sandbox requires to import modules in fixture to change os.environ
# imports in other test files will cause issues, so we need to test sandbox separately
- name: Run Tests Except Sandbox
run: poetry run pytest ./tests/unit --ignore ./tests/unit/test_sandbox.py
- name: Run Tests For Sandbox
run: poetry run pytest ./tests/unit/test_sandbox.py

test_matrix_success:
name: All Mac/Linux Tests Passed
Expand Down
15 changes: 15 additions & 0 deletions Development.md
Expand Up @@ -82,3 +82,18 @@ If you encounter any issues with the Language Model (LM) or you're simply curiou
```bash
make help
```

### 8. Testing

#### Unit tests

**NOTE**: we need to run unit tests for `test_sandbox.py` separately. `test_sandbox.py` need to import OpenDevin modules in the fixture Running it together with other file will cause it to re-use the imported module and loaded config from other modules, hence causing errors.

```bash
poetry run pytest ./tests/unit --ignore ./tests/unit/test_sandbox.py
poetry run pytest ./tests/unit/test_sandbox.py
```

#### Integration tests

Please refer to [this README](./tests/integration/README.md) for details.
2 changes: 1 addition & 1 deletion agenthub/monologue_agent/agent.py
Expand Up @@ -82,7 +82,7 @@
"I'll need a strategy. And as I make progress, I'll need to keep refining that strategy. I'll need to set goals, and break them into sub-goals.",
'In between actions, I must always take some time to think, strategize, and set new goals. I should never take two actions in a row.',
"OK so my task is to $TASK. I haven't made any progress yet. Where should I start?",
'It seems like there might be an existing project here. I should probably start by running `pwd` and `ls` to orient myself.',
"It seems like there might be an existing project here. I should probably start by running `ls` to see what's here.",
]


Expand Down
20 changes: 6 additions & 14 deletions agenthub/monologue_agent/utils/prompts.py
Expand Up @@ -28,8 +28,8 @@


Your most recent thought is at the bottom of that monologue. Continue your train of thought.
What is your next single thought or action? Your response must be in JSON format.
It must be a single object, and it must contain two fields:
What is your next thought or action? Your response must be in JSON format.
It must be an object, and it must contain two fields:
* `action`, which is one of the actions below
* `args`, which is a map of key-value pairs, specifying the arguments for that action

Expand Down Expand Up @@ -63,15 +63,11 @@
actions are all "think" actions, you should consider taking a different action.

Notes:
* you are logged in as %(user)s, but sudo will always work without a password.
* all non-background commands will be forcibly stopped if they remain running for over %(timeout)s seconds.
* your environment is Debian Linux. You can install software with `sudo apt-get`, but remember to use -y.
* your environment is Debian Linux. You can install software with `apt`
* your working directory will not change, even if you run `cd`. All commands will be run in the `%(WORKSPACE_MOUNT_PATH_IN_SANDBOX)s` directory.
* don't run interactive commands, or commands that don't return (e.g. `node server.js`). You may run commands in the background (e.g. `node server.js &`)
* don't run interactive text editors (e.g. `nano` or 'vim'), instead use the 'write' or 'read' action.
* don't run gui applications (e.g. software IDEs (like vs code or codium), web browsers (like firefox or chromium), or other complex software packages). Use non-interactive cli applications, or special actions instead.
* whenever an action fails, always `think` about why it may have happened before acting again.

What is your next single thought or action? Again, you must reply with JSON, and only with JSON. You must respond with exactly one 'action' object.
What is your next thought or action? Again, you must reply with JSON, and only with JSON.

%(hint)s
"""
Expand Down Expand Up @@ -150,15 +146,11 @@ def get_request_action_prompt(
)
bg_commands_message += '\nYou can end any process by sending a `kill` action with the numerical `id` above.'

user = 'opendevin' if config.get(ConfigType.RUN_AS_DEVIN) else 'root'

return ACTION_PROMPT % {
'task': task,
'monologue': json.dumps(thoughts, indent=2),
'background_commands': bg_commands_message,
'hint': hint,
'user': user,
'timeout': config.get(ConfigType.SANDBOX_TIMEOUT),
'WORKSPACE_MOUNT_PATH_IN_SANDBOX': config.get(ConfigType.WORKSPACE_MOUNT_PATH_IN_SANDBOX),
}

Expand Down Expand Up @@ -189,7 +181,7 @@ def rank(match):
raise LLMOutputError(
'Invalid JSON, the response must be well-formed JSON as specified in the prompt.'
)
except (ValueError, TypeError):
except ValueError:
raise LLMOutputError(
'Invalid JSON, the response must be well-formed JSON as specified in the prompt.'
)
Expand Down
5 changes: 2 additions & 3 deletions frontend/src/App.tsx
Expand Up @@ -9,14 +9,13 @@ import Workspace from "#/components/Workspace";
import LoadPreviousSessionModal from "#/components/modals/load-previous-session/LoadPreviousSessionModal";
import SettingsModal from "#/components/modals/settings/SettingsModal";
import { fetchMsgTotal } from "#/services/session";
import { initializeAgent } from "#/services/settingsService";
import Socket from "#/services/socket";
import { ResFetchMsgTotal } from "#/types/ResponseType";
import "./App.css";
import AgentControlBar from "./components/AgentControlBar";
import AgentStatusBar from "./components/AgentStatusBar";
import Terminal from "./components/terminal/Terminal";
import { initializeAgent } from "./services/agent";
import { getSettings } from "./services/settings";

interface Props {
setSettingOpen: (isOpen: boolean) => void;
Expand Down Expand Up @@ -73,7 +72,7 @@ function App(): JSX.Element {
if (initOnce) return;
initOnce = true;

initializeAgent(getSettings());
initializeAgent();

Socket.registerCallback("open", [getMsgTotal]);

Expand Down
9 changes: 0 additions & 9 deletions frontend/src/api/index.ts

This file was deleted.

36 changes: 0 additions & 36 deletions frontend/src/components/modals/base-modal/BaseModal.test.tsx
Expand Up @@ -96,42 +96,6 @@ describe("BaseModal", () => {
expect(screen.getByText("Children")).toBeInTheDocument();
});

it("should disable the action given the condition", () => {
const { rerender } = render(
<BaseModal
isOpen
onOpenChange={vi.fn}
title="Settings"
actions={[
{
label: "Save",
action: () => {},
isDisabled: true,
},
]}
/>,
);

expect(screen.getByText("Save")).toBeDisabled();

rerender(
<BaseModal
isOpen
onOpenChange={vi.fn}
title="Settings"
actions={[
{
label: "Save",
action: () => {},
isDisabled: false,
},
]}
/>,
);

expect(screen.getByText("Save")).not.toBeDisabled();
});

it.skip("should not close if the backdrop or escape key is pressed", () => {
const onOpenChangeMock = vi.fn();
render(
Expand Down
30 changes: 13 additions & 17 deletions frontend/src/components/modals/base-modal/FooterContent.tsx
Expand Up @@ -3,7 +3,6 @@ import React from "react";

export interface Action {
action: () => void;
isDisabled?: boolean;
label: string;
className?: string;
closeAfterAction?: boolean;
Expand All @@ -17,22 +16,19 @@ interface FooterContentProps {
export function FooterContent({ actions, closeModal }: FooterContentProps) {
return (
<>
{actions.map(
({ action, isDisabled, label, className, closeAfterAction }) => (
<Button
key={label}
type="button"
isDisabled={isDisabled}
onClick={() => {
action();
if (closeAfterAction) closeModal();
}}
className={className}
>
{label}
</Button>
),
)}
{actions.map(({ action, label, className, closeAfterAction }) => (
<Button
key={label}
type="button"
onClick={() => {
action();
if (closeAfterAction) closeModal();
}}
className={className}
>
{label}
</Button>
))}
</>
);
}
25 changes: 7 additions & 18 deletions frontend/src/components/modals/settings/SettingsForm.test.tsx
Expand Up @@ -4,22 +4,15 @@ import React from "react";
import { renderWithProviders } from "test-utils";
import AgentTaskState from "#/types/AgentTaskState";
import SettingsForm from "./SettingsForm";
import { Settings } from "#/services/settings";

const onModelChangeMock = vi.fn();
const onAgentChangeMock = vi.fn();
const onLanguageChangeMock = vi.fn();

const renderSettingsForm = (settings?: Settings) => {
const renderSettingsForm = (settings: Partial<Settings>) => {
renderWithProviders(
<SettingsForm
settings={
settings || {
LLM_MODEL: "model1",
AGENT: "agent1",
LANGUAGE: "en",
}
}
settings={settings}
models={["model1", "model2", "model3"]}
agents={["agent1", "agent2", "agent3"]}
onModelChange={onModelChangeMock}
Expand All @@ -31,7 +24,7 @@ const renderSettingsForm = (settings?: Settings) => {

describe("SettingsForm", () => {
it("should display the first values in the array by default", () => {
renderSettingsForm();
renderSettingsForm({});

const modelInput = screen.getByRole("combobox", { name: "model" });
const agentInput = screen.getByRole("combobox", { name: "agent" });
Expand Down Expand Up @@ -61,11 +54,7 @@ describe("SettingsForm", () => {
it("should disable settings while task is running", () => {
renderWithProviders(
<SettingsForm
settings={{
LLM_MODEL: "model1",
AGENT: "agent1",
LANGUAGE: "en",
}}
settings={{}}
models={["model1", "model2", "model3"]}
agents={["agent1", "agent2", "agent3"]}
onModelChange={onModelChangeMock}
Expand All @@ -85,7 +74,7 @@ describe("SettingsForm", () => {

describe("onChange handlers", () => {
it("should call the onModelChange handler when the model changes", () => {
renderSettingsForm();
renderSettingsForm({});

const modelInput = screen.getByRole("combobox", { name: "model" });
act(() => {
Expand All @@ -101,7 +90,7 @@ describe("SettingsForm", () => {
});

it("should call the onAgentChange handler when the agent changes", () => {
renderSettingsForm();
renderSettingsForm({});

const agentInput = screen.getByRole("combobox", { name: "agent" });
act(() => {
Expand All @@ -117,7 +106,7 @@ describe("SettingsForm", () => {
});

it("should call the onLanguageChange handler when the language changes", () => {
renderSettingsForm();
renderSettingsForm({});

const languageInput = screen.getByRole("combobox", { name: "language" });
act(() => {
Expand Down
3 changes: 1 addition & 2 deletions frontend/src/components/modals/settings/SettingsForm.tsx
Expand Up @@ -6,10 +6,9 @@ import { I18nKey } from "../../../i18n/declaration";
import { RootState } from "../../../store";
import AgentTaskState from "../../../types/AgentTaskState";
import { AutocompleteCombobox } from "./AutocompleteCombobox";
import { Settings } from "#/services/settings";

interface SettingsFormProps {
settings: Settings;
settings: Partial<Settings>;
models: string[];
agents: string[];

Expand Down