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

Fix tests #995

Merged
merged 19 commits into from
Dec 27, 2024
Merged

Fix tests #995

merged 19 commits into from
Dec 27, 2024

Conversation

whiterabbit1983
Copy link
Contributor

@whiterabbit1983 whiterabbit1983 commented Dec 26, 2024

PR Type

Tests, Bug fix


Description

Bug fixes and test improvements:

  • Fixed tools handling in task data conversion to properly handle empty lists and null values
  • Added connection pool support throughout the codebase for better database connection management
  • Improved chat context preparation with proper timestamp handling and null checks
  • Restored and updated numerous tests to work with the new connection pool architecture
  • Added proper error handling and skip decorators for incomplete tests
  • Updated CI workflows to include Go migrate installation

Changes walkthrough 📝

Relevant files
Bug fix
tasks.py
Fix tools handling in task data conversion                             

agents-api/agents_api/common/protocol/tasks.py

  • Fixed tools handling by ensuring empty lists are properly handled
  • Simplified tools list comprehension
  • +2/-4     
    prepare_chat_context.py
    Improve chat context preparation with timestamps and null checks

    agents-api/agents_api/queries/chat/prepare_chat_context.py

  • Added utcnow() for session updated_at timestamp
  • Added null check for tools
  • Fixed users list initialization
  • +7/-0     
    Enhancement
    gather_messages.py
    Add connection pool support to chat messages gathering     

    agents-api/agents_api/queries/chat/gather_messages.py

  • Added connection_pool parameter to gather_messages function
  • Updated function calls to pass connection_pool parameter
  • +6/-0     
    Tests
    test_chat_routes.py
    Restore and update chat route tests                                           

    agents-api/tests/test_chat_routes.py

  • Uncommented and fixed chat route tests
  • Updated tests to use connection pool instead of client
  • Added proper database connection handling
  • +185/-177
    test_execution_workflow.py
    Update execution workflow tests with connection pool         

    agents-api/tests/test_execution_workflow.py

  • Updated tests to use connection pool instead of client
  • Added skip decorators for incomplete tests
  • Fixed imports and test configurations
  • +925/-944

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information


    Important

    Enhance test handling, database connection management, and CI workflows with connection pool support, improved error handling, and updated tests.

    • Behavior:
      • Fix tools handling in tasks.py to handle empty lists and null values.
      • Add connection pool support in gather_messages.py and prepare_chat_context.py for better database management.
      • Improve timestamp handling and null checks in prepare_chat_context.py.
    • Tests:
      • Restore and update tests in test_chat_routes.py and test_execution_workflow.py to work with connection pool.
      • Add skip decorators for incomplete tests in test_execution_workflow.py.
    • CI/CD:
      • Update CI workflows (lint-agents-api-pr.yml, test-agents-api-pr.yml, typecheck-agents-api-pr.yml) to include Go migrate installation.
    • Configuration:
      • Update ruff.toml for linting and formatting settings.

    This description was created by Ellipsis for 2a186ba. It will automatically update as commits are pushed.

    Copy link

    gitguardian bot commented Dec 26, 2024

    ️✅ There are no secrets present in this pull request anymore.

    If these secrets were true positive and are still valid, we highly recommend you to revoke them.
    While these secrets were previously flagged, we no longer have a reference to the
    specific commits where they were detected. Once a secret has been leaked into a git
    repository, you should consider it compromised, even if it was deleted immediately.
    Find here more information about risks.


    🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

    Copy link

    qodo-merge-pro-for-open-source bot commented Dec 26, 2024

    CI Failure Feedback 🧐

    (Checks updated until commit 2f90cb3)

    Action: Lint-And-Format

    Failed stage: Lint and format [❌]

    Failed test name: ruff check

    Failure summary:

    The action failed due to two Ruff linting errors related to implicit namespace packages:
    1. Missing
    init.py file in integrations/utils/ directory
    2. Nested package issue in
    integrations/utils/integrations/init.py requiring an init.py in the parent directory

    Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    319:  ##[endgroup]
    320:  �[37mPoe =>�[0m �[94mruff format�[0m
    321:  66 files left unchanged
    322:  �[37mPoe =>�[0m �[94mruff check�[0m
    323:  integrations/utils/execute_integration.py:
    324:  1:1 INP001 File `integrations/utils/execute_integration.py` is part of an implicit namespace package. Add an `__init__.py`.
    325:  integrations/utils/integrations/__init__.py:
    326:  1:1 INP001 File `integrations/utils/integrations/__init__.py` declares a package, but is nested under an implicit namespace package. Add an `__init__.py` to `integrations/utils`.
    327:  Found 2 errors.
    328:  ##[error]Process completed with exit code 1.
    ...
    
    330:  [command]/usr/bin/git version
    331:  git version 2.47.1
    332:  Temporarily overriding HOME='/home/runner/work/_temp/62a820e0-8fa1-4b68-be09-d44a01f20b62' before making global git config changes
    333:  Adding repository directory to the temporary git global config as a safe directory
    334:  [command]/usr/bin/git config --global --add safe.directory /home/runner/work/julep/julep
    335:  [command]/usr/bin/git config --local --name-only --get-regexp core\.sshCommand
    336:  [command]/usr/bin/git submodule foreach --recursive sh -c "git config --local --name-only --get-regexp 'core\.sshCommand' && git config --local --unset-all 'core.sshCommand' || :"
    337:  fatal: No url found for submodule path 'drafts/cozo' in .gitmodules
    338:  ##[warning]The process '/usr/bin/git' failed with exit code 128
    

    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"
    

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    @whiterabbit1983 whiterabbit1983 force-pushed the x/fix-tests branch 2 times, most recently from cfa1bef to 9ef620f Compare December 27, 2024 08:42
    @whiterabbit1983 whiterabbit1983 marked this pull request as ready for review December 27, 2024 08:44

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Test Coverage

    Many test cases are marked with @Skip decorator, which means important functionality is not being tested. The skipped tests should be fixed and enabled to ensure proper test coverage.

    @skip
    @test("workflow: evaluate step single")
    async def _(
        dsn=pg_dsn,
        developer_id=test_developer_id,
        agent=test_agent,
        _s3_client=s3_client,  # Adding coz blob store might be used
    ):
        pool = await create_db_pool(dsn=dsn)
        data = CreateExecutionRequest(input={"test": "input"})
    
        task = await create_task(
            developer_id=developer_id,
            agent_id=agent.id,
            data=CreateTaskRequest(
                **{
                    "name": "test task",
                    "description": "test task about",
                    "input_schema": {"type": "object", "additionalProperties": True},
                    "main": [{"evaluate": {"hello": '"world"'}}],
                }
            ),
            connection_pool=pool,
        )
    
        async with patch_testing_temporal() as (_, mock_run_task_execution_workflow):
            execution, handle = await start_execution(
                developer_id=developer_id,
                task_id=task.id,
                data=data,
                connection_pool=pool,
            )
    
            assert handle is not None
            assert execution.task_id == task.id
            assert execution.input == data.input
            mock_run_task_execution_workflow.assert_called_once()
    
            try:
                result = await handle.result()
                assert result["hello"] == "world"
            except Exception as ex:
                breakpoint()
                raise ex
    
    
    @skip
    @test("workflow: evaluate step multiple")
    async def _(
        dsn=pg_dsn,
        developer_id=test_developer_id,
        agent=test_agent,
    ):
        pool = await create_db_pool(dsn=dsn)
        data = CreateExecutionRequest(input={"test": "input"})
    
        task = create_task(
            developer_id=developer_id,
            agent_id=agent.id,
            data=CreateTaskRequest(
                **{
                    "name": "test task",
                    "description": "test task about",
                    "input_schema": {"type": "object", "additionalProperties": True},
                    "main": [
                        {"evaluate": {"hello": '"nope"'}},
                        {"evaluate": {"hello": '"world"'}},
                    ],
                }
            ),
            connection_pool=pool,
        )
    
        async with patch_testing_temporal() as (_, mock_run_task_execution_workflow):
            execution, handle = await start_execution(
                developer_id=developer_id,
                task_id=task.id,
                data=data,
                connection_pool=pool,
            )
    
            assert handle is not None
            assert execution.task_id == task.id
            assert execution.input == data.input
            mock_run_task_execution_workflow.assert_called_once()
    
            result = await handle.result()
            assert result["hello"] == "world"
    
    
    @skip
    @test("workflow: variable access in expressions")
    async def _(
        dsn=pg_dsn,
        developer_id=test_developer_id,
        agent=test_agent,
    ):
        pool = await create_db_pool(dsn=dsn)
        data = CreateExecutionRequest(input={"test": "input"})
    
        task = create_task(
            developer_id=developer_id,
            agent_id=agent.id,
            data=CreateTaskRequest(
                **{
                    "name": "test task",
                    "description": "test task about",
                    "input_schema": {"type": "object", "additionalProperties": True},
                    "main": [
                        # Testing that we can access the input
                        {"evaluate": {"hello": '_["test"]'}},
                    ],
                }
            ),
            connection_pool=pool,
        )
    
        async with patch_testing_temporal() as (_, mock_run_task_execution_workflow):
            execution, handle = await start_execution(
                developer_id=developer_id,
                task_id=task.id,
                data=data,
                connection_pool=pool,
            )
    
            assert handle is not None
            assert execution.task_id == task.id
            assert execution.input == data.input
            mock_run_task_execution_workflow.assert_called_once()
    
            result = await handle.result()
            assert result["hello"] == data.input["test"]
    
    
    @skip
    @test("workflow: yield step")
    async def _(
        dsn=pg_dsn,
        developer_id=test_developer_id,
        agent=test_agent,
    ):
        pool = await create_db_pool(dsn=dsn)
        data = CreateExecutionRequest(input={"test": "input"})
    
        task = create_task(
            developer_id=developer_id,
            agent_id=agent.id,
            data=CreateTaskRequest(
                **{
                    "name": "test task",
                    "description": "test task about",
                    "input_schema": {"type": "object", "additionalProperties": True},
                    "other_workflow": [
                        # Testing that we can access the input
                        {"evaluate": {"hello": '_["test"]'}},
                    ],
                    "main": [
                        # Testing that we can access the input
                        {
                            "workflow": "other_workflow",
                            "arguments": {"test": '_["test"]'},
                        },
                    ],
                }
            ),
            connection_pool=pool,
        )
    
        async with patch_testing_temporal() as (_, mock_run_task_execution_workflow):
            execution, handle = await start_execution(
                developer_id=developer_id,
                task_id=task.id,
                data=data,
                connection_pool=pool,
            )
    
            assert handle is not None
            assert execution.task_id == task.id
            assert execution.input == data.input
            mock_run_task_execution_workflow.assert_called_once()
    
            result = await handle.result()
            assert result["hello"] == data.input["test"]
    
    
    @skip
    @test("workflow: sleep step")
    async def _(
        dsn=pg_dsn,
        developer_id=test_developer_id,
        agent=test_agent,
    ):
        pool = await create_db_pool(dsn=dsn)
        data = CreateExecutionRequest(input={"test": "input"})
    
        task = create_task(
            developer_id=developer_id,
            agent_id=agent.id,
            data=CreateTaskRequest(
                **{
                    "name": "test task",
                    "description": "test task about",
                    "input_schema": {"type": "object", "additionalProperties": True},
                    "other_workflow": [
                        # Testing that we can access the input
                        {"evaluate": {"hello": '_["test"]'}},
                        {"sleep": {"days": 5}},
                    ],
                    "main": [
                        # Testing that we can access the input
                        {
                            "workflow": "other_workflow",
                            "arguments": {"test": '_["test"]'},
                        },
                    ],
                }
            ),
            connection_pool=pool,
        )
    
        async with patch_testing_temporal() as (_, mock_run_task_execution_workflow):
            execution, handle = await start_execution(
                developer_id=developer_id, task_id=task.id, data=data, connection_pool=pool
            )
    
            assert handle is not None
            assert execution.task_id == task.id
            assert execution.input == data.input
            mock_run_task_execution_workflow.assert_called_once()
    
            result = await handle.result()
            assert result["hello"] == data.input["test"]
    
    
    @skip
    @test("workflow: return step direct")
    async def _(
        dsn=pg_dsn,
        developer_id=test_developer_id,
        agent=test_agent,
    ):
        pool = await create_db_pool(dsn=dsn)
        data = CreateExecutionRequest(input={"test": "input"})
    
        task = create_task(
            developer_id=developer_id,
            agent_id=agent.id,
            data=CreateTaskRequest(
                **{
                    "name": "test task",
                    "description": "test task about",
                    "input_schema": {"type": "object", "additionalProperties": True},
                    "main": [
                        # Testing that we can access the input
                        {"evaluate": {"hello": '_["test"]'}},
                        {"return": {"value": '_["hello"]'}},
                        {"return": {"value": '"banana"'}},
                    ],
                }
            ),
            connection_pool=pool,
        )
    
        async with patch_testing_temporal() as (_, mock_run_task_execution_workflow):
            execution, handle = await start_execution(
                developer_id=developer_id, task_id=task.id, data=data, connection_pool=pool
            )
    
            assert handle is not None
            assert execution.task_id == task.id
            assert execution.input == data.input
            mock_run_task_execution_workflow.assert_called_once()
    
            result = await handle.result()
            assert result["value"] == data.input["test"]
    
    
    @skip
    @test("workflow: return step nested")
    async def _(
        dsn=pg_dsn,
        developer_id=test_developer_id,
        agent=test_agent,
    ):
        pool = await create_db_pool(dsn=dsn)
        data = CreateExecutionRequest(input={"test": "input"})
    
        task = create_task(
            developer_id=developer_id,
            agent_id=agent.id,
            data=CreateTaskRequest(
                **{
                    "name": "test task",
                    "description": "test task about",
                    "input_schema": {"type": "object", "additionalProperties": True},
                    "other_workflow": [
                        # Testing that we can access the input
                        {"evaluate": {"hello": '_["test"]'}},
                        {"return": {"value": '_["hello"]'}},
                        {"return": {"value": '"banana"'}},
                    ],
                    "main": [
                        # Testing that we can access the input
                        {
                            "workflow": "other_workflow",
                            "arguments": {"test": '_["test"]'},
                        },
                    ],
                }
            ),
            connection_pool=pool,
        )
    
        async with patch_testing_temporal() as (_, mock_run_task_execution_workflow):
            execution, handle = await start_execution(
                developer_id=developer_id, task_id=task.id, data=data, connection_pool=pool
            )
    
            assert handle is not None
            assert execution.task_id == task.id
            assert execution.input == data.input
            mock_run_task_execution_workflow.assert_called_once()
    
            result = await handle.result()
            assert result["value"] == data.input["test"]
    
    
    @skip
    @test("workflow: log step")
    async def _(
        dsn=pg_dsn,
        developer_id=test_developer_id,
        agent=test_agent,
    ):
        pool = await create_db_pool(dsn=dsn)
        data = CreateExecutionRequest(input={"test": "input"})
    
        task = create_task(
            developer_id=developer_id,
            agent_id=agent.id,
            data=CreateTaskRequest(
                **{
                    "name": "test task",
                    "description": "test task about",
                    "input_schema": {"type": "object", "additionalProperties": True},
                    "other_workflow": [
                        # Testing that we can access the input
                        {"evaluate": {"hello": '_["test"]'}},
                        {"log": "{{_.hello}}"},
                    ],
                    "main": [
                        # Testing that we can access the input
                        {
                            "workflow": "other_workflow",
                            "arguments": {"test": '_["test"]'},
                        },
                    ],
                }
            ),
            connection_pool=pool,
        )
    
        async with patch_testing_temporal() as (_, mock_run_task_execution_workflow):
            execution, handle = await start_execution(
                developer_id=developer_id, task_id=task.id, data=data, connection_pool=pool
            )
    
            assert handle is not None
            assert execution.task_id == task.id
            assert execution.input == data.input
            mock_run_task_execution_workflow.assert_called_once()
    
            result = await handle.result()
            assert result["hello"] == data.input["test"]
    
    
    @skip
    @test("workflow: log step expression fail")
    async def _(
        dsn=pg_dsn,
        developer_id=test_developer_id,
        agent=test_agent,
    ):
        pool = await create_db_pool(dsn=dsn)
        data = CreateExecutionRequest(input={"test": "input"})
    
        task = create_task(
            developer_id=developer_id,
            agent_id=agent.id,
            data=CreateTaskRequest(
                **{
                    "name": "test task",
                    "description": "test task about",
                    "input_schema": {"type": "object", "additionalProperties": True},
                    "other_workflow": [
                        # Testing that we can access the input
                        {"evaluate": {"hello": '_["test"]'}},
                        {
                            "log": '{{_["hell"].strip()}}'
                        },  # <--- The "hell" key does not exist
                    ],
                    "main": [
                        # Testing that we can access the input
                        {
                            "workflow": "other_workflow",
                            "arguments": {"test": '_["test"]'},
                        },
                    ],
                }
            ),
            connection_pool=pool,
        )
    
        async with patch_testing_temporal() as (_, mock_run_task_execution_workflow):
            with raises(BaseException):
                execution, handle = await start_execution(
                    developer_id=developer_id,
                    task_id=task.id,
                    data=data,
                    connection_pool=pool,
                )
    
                assert handle is not None
                assert execution.task_id == task.id
                assert execution.input == data.input
                mock_run_task_execution_workflow.assert_called_once()
    
                result = await handle.result()
                assert result["hello"] == data.input["test"]
    
    
    @skip
    @test("workflow: system call - list agents")
    async def _(
        dsn=pg_dsn,
        developer_id=test_developer_id,
        agent=test_agent,
    ):
        pool = await create_db_pool(dsn=dsn)
        data = CreateExecutionRequest(input={})
    
        task = create_task(
            developer_id=developer_id,
            agent_id=agent.id,
            data=CreateTaskRequest(
                **{
                    "name": "Test system tool task",
                    "description": "List agents using system call",
                    "input_schema": {"type": "object"},
                    "tools": [
                        {
                            "name": "list_agents",
                            "description": "List all agents",
                            "type": "system",
                            "system": {"resource": "agent", "operation": "list"},
                        },
                    ],
                    "main": [
                        {
                            "tool": "list_agents",
                            "arguments": {
                                "limit": "10",
                            },
                        },
                    ],
                }
            ),
            connection_pool=pool,
        )
    
        async with patch_testing_temporal() as (_, mock_run_task_execution_workflow):
            execution, handle = await start_execution(
                developer_id=developer_id, task_id=task.id, data=data, connection_pool=pool
            )
    
            assert handle is not None
            assert execution.task_id == task.id
            assert execution.input == data.input
            mock_run_task_execution_workflow.assert_called_once()
    
            result = await handle.result()
            assert isinstance(result, list)
            # Result's length should be less than or equal to the limit
            assert len(result) <= 10
            # Check if all items are agent dictionaries
            assert all(isinstance(agent, dict) for agent in result)
            # Check if each agent has an 'id' field
            assert all("id" in agent for agent in result)
    
    
    @skip
    @test("workflow: tool call api_call")
    async def _(
        dsn=pg_dsn,
        developer_id=test_developer_id,
        agent=test_agent,
    ):
        pool = await create_db_pool(dsn=dsn)
        data = CreateExecutionRequest(input={"test": "input"})
    
        task = create_task(
            developer_id=developer_id,
            agent_id=agent.id,
            data=CreateTaskRequest(
                **{
                    "name": "test task",
                    "description": "test task about",
                    "input_schema": {"type": "object", "additionalProperties": True},
                    "tools": [
                        {
                            "type": "api_call",
                            "name": "hello",
                            "api_call": {
                                "method": "GET",
                                "url": "https://httpbin.org/get",
                            },
                        }
                    ],
                    "main": [
                        {
                            "tool": "hello",
                            "arguments": {
                                "params": {"test": "_.test"},
                            },
                        },
                        {
                            "evaluate": {"hello": "_.json.args.test"},
                        },
                    ],
                }
            ),
            connection_pool=pool,
        )
    
        async with patch_testing_temporal() as (_, mock_run_task_execution_workflow):
            execution, handle = await start_execution(
                developer_id=developer_id, task_id=task.id, data=data, connection_pool=pool
            )
    
            assert handle is not None
            assert execution.task_id == task.id
            assert execution.input == data.input
            mock_run_task_execution_workflow.assert_called_once()
    
            result = await handle.result()
            assert result["hello"] == data.input["test"]
    Test Reliability

    Some tests are noted as failing randomly in CI or requiring sleep delays. These tests should be made more robust and deterministic to prevent flaky test behavior.

    # TODO: Fix this test. It fails sometimes and sometimes not.
    @test("route: search agent docs")
    async def _(make_request=make_request, agent=test_agent, doc=test_doc):
        time.sleep(0.5)
        search_params = dict(
            text=doc.content[0],
            limit=1,
        )
    
        response = make_request(
            method="POST",
            url=f"/agents/{agent.id}/search",
            json=search_params,
        )
    
        assert response.status_code == 200
        response = response.json()
        docs = response["docs"]
    
        assert isinstance(docs, list)
        assert len(docs) >= 1
    
    
    # FIXME: This test is failing because the search is not returning the expected results
    @skip("Fails randomly on CI")
    @test("route: search user docs")
    async def _(make_request=make_request, user=test_user, doc=test_user_doc):
        time.sleep(0.5)
        search_params = dict(
            text=doc.content[0],
            limit=1,
        )
    
        response = make_request(
            method="POST",
            url=f"/users/{user.id}/search",
            json=search_params,
        )
    
        assert response.status_code == 200
        response = response.json()
        docs = response["docs"]
    
        assert isinstance(docs, list)
    
        assert len(docs) >= 1
    
    
    @test("route: search agent docs hybrid with mmr")
    async def _(make_request=make_request, agent=test_agent, doc=test_doc):
        time.sleep(0.5)
    
        EMBEDDING_SIZE = 1024
        search_params = dict(
            text=doc.content[0],
            vector=[1.0] * EMBEDDING_SIZE,
            mmr_strength=0.5,
            limit=1,
        )
    
        response = make_request(
            method="POST",
            url=f"/agents/{agent.id}/search",
            json=search_params,
        )
    
        assert response.status_code == 200
        response = response.json()
        docs = response["docs"]
    
        assert isinstance(docs, list)
        assert len(docs) >= 1

    Copy link

    qodo-merge-pro-for-open-source bot commented Dec 27, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Timestamp updates should be handled by session management code rather than context preparation

    The session's updated_at timestamp should be set only when actually updating the
    session, not during context preparation. Move this logic to the session update
    handler.

    agents-api/agents_api/queries/chat/prepare_chat_context.py [124]

    -d["session"]["updated_at"] = utcnow()
    +# Remove this line and handle updated_at in session update logic
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Moving timestamp updates to session management is a critical architectural improvement that maintains proper separation of concerns and prevents potential race conditions.

    8
    Replace fixed delays with proper retry logic in tests to handle timing-dependent operations

    Add error handling and retries for the flaky search test instead of using
    time.sleep(), which is unreliable and can still fail.

    agents-api/tests/test_docs_routes.py [174-176]

     @test("route: search agent docs")
     async def _(make_request=make_request, agent=test_agent, doc=test_doc):
    -    time.sleep(0.5)
    +    max_retries = 3
    +    for _ in range(max_retries):
    +        try:
    +            # Test code here
    +            break
    +        except AssertionError:
    +            await asyncio.sleep(0.5)
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion improves test reliability by replacing a fixed sleep with a proper retry mechanism, making tests more robust and less likely to fail intermittently.

    6
    Add descriptive reasons when skipping tests to improve test maintenance and documentation

    The test function is marked with @Skip but lacks a reason for skipping. Add a
    descriptive reason in the skip decorator to document why the test is currently
    disabled.

    agents-api/tests/test_execution_workflow.py [71-73]

    -@skip
    +@skip("Test needs to be updated for new connection pool implementation")
     @test("workflow: evaluate step multiple")
     async def _(
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Adding skip reasons improves test maintainability by documenting why tests are disabled, though it's a minor enhancement that doesn't affect functionality.

    5
    Possible issue
    ✅ Use specific exception handling instead of bare except clauses to avoid masking errors
    Suggestion Impact:The commit changes the exception handling from BaseException to TimeoutError in the test function

    code diff:

                 await asyncio.wait_for(task, timeout=3)
    -        except asyncio.TimeoutError:
    +        except TimeoutError:

    The test function uses a bare except clause which could mask important errors.
    Replace with specific exception handling.

    agents-api/tests/test_execution_workflow.py [629-631]

     try:
         await asyncio.wait_for(task, timeout=10)
    -except BaseException:
    +except asyncio.TimeoutError:
         task.cancel()
    +except Exception as e:
    +    task.cancel()
    +    raise e
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using specific exception handling prevents masking potential errors and improves error handling robustness. The current bare except could hide important issues.

    7
    Add proper null checking for optional model attributes to prevent runtime errors

    Add validation to ensure recall_options is properly handled when null, as the
    current code may raise an attribute error if data.recall_options is None.

    agents-api/agents_api/queries/sessions/create_session.py [141]

    -data.recall_options.model_dump() if data.recall_options else {},  # $10
    +data.recall_options.model_dump() if data.recall_options is not None else {},  # $10
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion addresses a potential runtime error by adding explicit None checking, improving code robustness and preventing potential crashes.

    7

    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    👍 Looks good to me! Reviewed everything up to 3a44a80 in 1 minute and 26 seconds

    More details
    • Looked at 3690 lines of code in 18 files
    • Skipped 0 files when reviewing.
    • Skipped posting 7 drafted comments based on config settings.
    1. .github/workflows/lint-agents-api-pr.yml:27
    • Draft comment:
      The installation of Go migrate is added, but it is not used in this workflow. Consider removing it if it's not needed.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The PR adds installation of Go migrate in multiple workflow files. However, the installation is not used anywhere in the workflows. This might be unnecessary unless there's a future plan to use it.
    2. .github/workflows/test-agents-api-pr.yml:27
    • Draft comment:
      The installation of Go migrate is added, but it is not used in this workflow. Consider removing it if it's not needed.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The PR adds installation of Go migrate in multiple workflow files. However, the installation is not used anywhere in the workflows. This might be unnecessary unless there's a future plan to use it.
    3. .github/workflows/typecheck-agents-api-pr.yml:35
    • Draft comment:
      The installation of Go migrate is added, but it is not used in this workflow. Consider removing it if it's not needed.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The PR adds installation of Go migrate in multiple workflow files. However, the installation is not used anywhere in the workflows. This might be unnecessary unless there's a future plan to use it.
    4. agents-api/agents_api/common/protocol/tasks.py:313
    • Draft comment:
      The use of or [] is a good change to handle cases where tools might be None. This makes the code cleaner and more readable.
    • Reason this comment was not posted:
      Confidence changes required: 0%
      The code in spec_to_task_data function has been simplified by using or [] to handle the case where tools might be None. This is a good change as it makes the code cleaner and more readable.
    5. agents-api/agents_api/queries/chat/gather_messages.py:38
    • Draft comment:
      The addition of connection_pool parameter is a good change for better resource management and performance.
    • Reason this comment was not posted:
      Confidence changes required: 0%
      The connection_pool parameter is added to multiple function calls. This is a good change as it allows for better resource management and potentially improved performance by reusing connections.
    6. agents-api/agents_api/queries/sessions/create_session.py:141
    • Draft comment:
      The addition of connection_pool parameter is a good change for better resource management and performance.
    • Reason this comment was not posted:
      Confidence changes required: 0%
      The connection_pool parameter is added to multiple function calls. This is a good change as it allows for better resource management and potentially improved performance by reusing connections.
    7. agents-api/agents_api/routers/sessions/chat.py:221
    • Draft comment:
      The removal of mark_session_as_updated parameter might be intentional if it's no longer needed. Ensure that session updates are handled appropriately elsewhere if required.
    • Reason this comment was not posted:
      Confidence changes required: 33%
      The mark_session_as_updated parameter is removed from the create_entries function call. This might be intentional if the parameter is no longer needed or handled differently.

    Workflow ID: wflow_WkZ2wFOcgmdSF9cs


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    👍 Looks good to me! Incremental review on 273357d in 33 seconds

    More details
    • Looked at 92 lines of code in 1 files
    • Skipped 0 files when reviewing.
    • Skipped posting 1 drafted comments based on config settings.
    1. agents-api/tests/test_docs_routes.py:17
    • Draft comment:
      The @skip decorator is used extensively in this file, which means many tests are being skipped. Ensure this is intentional and that important tests are not being unintentionally skipped. Consider adding comments to explain why each test is skipped.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The use of the @skip decorator is prevalent throughout the test file, indicating that many tests are being skipped. This could be intentional for tests that are not ready or are known to fail, but it should be documented or reviewed to ensure that important tests are not being unintentionally skipped.

    Workflow ID: wflow_oUazTT0gsXPe6oix


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    👍 Looks good to me! Incremental review on c7a085a in 47 seconds

    More details
    • Looked at 243 lines of code in 2 files
    • Skipped 0 files when reviewing.
    • Skipped posting 7 drafted comments based on config settings.
    1. agents-api/tests/test_execution_workflow.py:110
    • Draft comment:
      Missing 'await' keyword before 'create_task'.
    • Reason this comment was not posted:
      Comment was not on a valid diff hunk.
    2. agents-api/tests/test_execution_workflow.py:153
    • Draft comment:
      Missing 'await' keyword before 'create_task'.
    • Reason this comment was not posted:
      Marked as duplicate.
    3. agents-api/tests/test_execution_workflow.py:203
    • Draft comment:
      Missing 'await' keyword before 'create_task'.
    • Reason this comment was not posted:
      Marked as duplicate.
    4. agents-api/tests/test_execution_workflow.py:251
    • Draft comment:
      Missing 'await' keyword before 'create_task'.
    • Reason this comment was not posted:
      Marked as duplicate.
    5. agents-api/tests/test_execution_workflow.py:293
    • Draft comment:
      Missing 'await' keyword before 'create_task'.
    • Reason this comment was not posted:
      Marked as duplicate.
    6. agents-api/tests/test_execution_workflow.py:342
    • Draft comment:
      Missing 'await' keyword before 'create_task'.
    • Reason this comment was not posted:
      Marked as duplicate.
    7. agents-api/tests/test_execution_workflow.py:390
    • Draft comment:
      Missing 'await' keyword before 'create_task'.
    • Reason this comment was not posted:
      Marked as duplicate.

    Workflow ID: wflow_TWtheifsTAjLadDo


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    👍 Looks good to me! Incremental review on fece441 in 35 seconds

    More details
    • Looked at 243 lines of code in 2 files
    • Skipped 0 files when reviewing.
    • Skipped posting 2 drafted comments based on config settings.
    1. agents-api/tests/test_docs_routes.py:177
    • Draft comment:
      Consider using await asyncio.sleep(0.5) instead of time.sleep(0.5) to avoid blocking the event loop in an async function.
    • Reason this comment was not posted:
      Comment was on unchanged code.
    2. agents-api/tests/test_docs_routes.py:224
    • Draft comment:
      Consider using await asyncio.sleep(0.5) instead of time.sleep(0.5) to avoid blocking the event loop in an async function.
    • Reason this comment was not posted:
      Marked as duplicate.

    Workflow ID: wflow_5sPtL8PXfyZHDMg2


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    👍 Looks good to me! Incremental review on ea37837 in 38 seconds

    More details
    • Looked at 99 lines of code in 3 files
    • Skipped 0 files when reviewing.
    • Skipped posting 5 drafted comments based on config settings.
    1. agents-api/agents_api/routers/docs/search_docs.py:31
    • Draft comment:
      The change from await search_docs_by_text to search_docs_by_text is correct here since search_docs_by_text is not awaited in this context. Ensure that the function is correctly awaited where it is called.
    • Reason this comment was not posted:
      Confidence changes required: 20%
      The change from 'await search_docs_by_text' to 'search_docs_by_text' in the search_docs.py file is correct because the function is not awaited in the context it is used. However, the call to 'search_fn' in 'search_user_docs' and 'search_agent_docs' should be awaited as it is an async function.
    2. agents-api/agents_api/routers/docs/search_docs.py:47
    • Draft comment:
      The change from await search_docs_by_embedding to search_docs_by_embedding is correct here since search_docs_by_embedding is not awaited in this context. Ensure that the function is correctly awaited where it is called.
    • Reason this comment was not posted:
      Confidence changes required: 20%
      The change from 'await search_docs_by_embedding' to 'search_docs_by_embedding' in the search_docs.py file is correct because the function is not awaited in the context it is used. However, the call to 'search_fn' in 'search_user_docs' and 'search_agent_docs' should be awaited as it is an async function.
    3. agents-api/agents_api/routers/docs/search_docs.py:63
    • Draft comment:
      The change from await search_docs_hybrid to search_docs_hybrid is correct here since search_docs_hybrid is not awaited in this context. Ensure that the function is correctly awaited where it is called.
    • Reason this comment was not posted:
      Confidence changes required: 20%
      The change from 'await search_docs_hybrid' to 'search_docs_hybrid' in the search_docs.py file is correct because the function is not awaited in the context it is used. However, the call to 'search_fn' in 'search_user_docs' and 'search_agent_docs' should be awaited as it is an async function.
    4. agents-api/agents_api/routers/docs/search_docs.py:100
    • Draft comment:
      The call to search_fn should be awaited here as it is an async function. This ensures proper execution and handling of asynchronous operations.
    • Reason this comment was not posted:
      Comment looked like it was already resolved.
    5. agents-api/agents_api/routers/docs/search_docs.py:151
    • Draft comment:
      The call to search_fn should be awaited here as it is an async function. This ensures proper execution and handling of asynchronous operations.
    • Reason this comment was not posted:
      Marked as duplicate.

    Workflow ID: wflow_x0E7DINeyw9GLyR6


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    👍 Looks good to me! Incremental review on f13ab99 in 27 seconds

    More details
    • Looked at 24 lines of code in 1 files
    • Skipped 0 files when reviewing.
    • Skipped posting 0 drafted comments based on config settings.

    Workflow ID: wflow_Vjlm0XJsBIGLKKp5


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    👍 Looks good to me! Incremental review on 505a25d in 39 seconds

    More details
    • Looked at 739 lines of code in 18 files
    • Skipped 0 files when reviewing.
    • Skipped posting 1 drafted comments based on config settings.
    1. agents-api/agents_api/activities/task_steps/base_evaluate.py:13
    • Draft comment:
      Remove the unused import testing from ...env.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The code in agents-api/agents_api/activities/task_steps/base_evaluate.py has a redundant import of testing from ...env which is not used anywhere in the file. Removing unused imports is a good practice to keep the code clean and efficient.

    Workflow ID: wflow_Y89x55cCu7YPLlIb


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    👍 Looks good to me! Incremental review on 30b2a92 in 4 minutes and 21 seconds

    More details
    • Looked at 13146 lines of code in 188 files
    • Skipped 0 files when reviewing.
    • Skipped posting 3 drafted comments based on config settings.
    1. ruff.toml:54
    • Draft comment:
      Consider adding the **/autogen/*.py pattern to the exclude list to avoid linting generated files.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The ruff.toml file is well-structured, but there are a few minor improvements that can be made for clarity and consistency.
    2. ruff.toml:57
    • Draft comment:
      Consider enabling the additional categories (C09, S, B, ARG, PTH, ERA, PLW, FURB) in the select list to cover more linting rules.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The ruff.toml file is well-structured, but there are a few minor improvements that can be made for clarity and consistency.
    3. ruff.toml:65
    • Draft comment:
      Ensure that the ignored rules (COM812, ISC001) are indeed conflicting and necessary to ignore, as ignoring them might overlook potential issues.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The ruff.toml file is well-structured, but there are a few minor improvements that can be made for clarity and consistency.

    Workflow ID: wflow_c44kZ90pnn5aqcHC


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    Signed-off-by: Diwank Singh Tomer <[email protected]>
    @creatorrr creatorrr merged commit 33ac790 into f/switch-to-pg Dec 27, 2024
    11 of 15 checks passed
    @creatorrr creatorrr deleted the x/fix-tests branch December 27, 2024 17:37
    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    👍 Looks good to me! Incremental review on 2f90cb3 in 4 minutes and 18 seconds

    More details
    • Looked at 14819 lines of code in 229 files
    • Skipped 0 files when reviewing.
    • Skipped posting 4 drafted comments based on config settings.
    1. integrations-service/integrations/utils/integrations/ffmpeg.py:140
    • Draft comment:
      Initialize temp_dir to None before the try block to avoid potential issues if an exception occurs before temp_dir is defined.
            temp_dir = None
    
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The bash_cmd function in ffmpeg.py has a potential issue with the temp_dir variable. If an exception occurs before temp_dir is defined, the cleanup code will raise an additional exception. This can be avoided by initializing temp_dir to None and checking if it is not None before attempting to remove it.
    2. integrations-service/integrations/routers/integrations/get_integrations.py:6
    • Draft comment:
      Specify the types of keys and values in the dictionary for better clarity and type checking.
    async def get_integrations() -> list[dict[str, Any]]:
    
    • Reason this comment was not posted:
      Confidence changes required: 30%
      In get_integrations.py, the function get_integrations is returning a list of dictionaries. However, the return type hint is list[dict], which is not specific. It would be better to specify the types of keys and values in the dictionary for better clarity and type checking.
    3. integrations-service/integrations/autogen/Sessions.py:34
    • Draft comment:
      Consider extracting the long default string for system_template into a constant or a separate function to improve readability and maintainability.
    • Reason this comment was not posted:
      Confidence changes required: 30%
      In Sessions.py, the system_template field has a very long default string value. This could be extracted into a constant or a separate function to improve readability and maintainability.
    4. ruff.toml:30
    • Draft comment:
      Ensure that enabling fix and unsafe-fixes is intentional and that the team is aware of the potential risks of unexpected changes.
    • Reason this comment was not posted:
      Confidence changes required: 20%
      In ruff.toml, the fix and unsafe-fixes options are set to true. While this can be useful, it might lead to unexpected changes. It's important to ensure that these settings are intentional and that the team is aware of the potential risks.

    Workflow ID: wflow_3iEli6AZFOTnzi8y


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    👍 Looks good to me! Incremental review on 2a186ba in 4 minutes and 55 seconds

    More details
    • Looked at 14819 lines of code in 229 files
    • Skipped 0 files when reviewing.
    • Skipped posting 1 drafted comments based on config settings.
    1. integrations-service/integrations/utils/integrations/remote_browser.py:369
    • Draft comment:
      Consider raising a more specific exception type instead of a generic Exception to provide more context about the error.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      In integrations-service/integrations/utils/integrations/remote_browser.py, the perform_action function has a try-except block that raises a generic Exception with a message. It would be better to raise a more specific exception type if possible.

    Workflow ID: wflow_e3v7VS1B9op2Z4lv


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants