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

chore: misc test and queries fixes #1001

Merged
merged 5 commits into from
Dec 31, 2024
Merged

Conversation

Vedantsahai18
Copy link
Member

@Vedantsahai18 Vedantsahai18 commented Dec 28, 2024

PR Type

Bug fix, Enhancement


Description

  • Fixed SQL syntax issues in task queries by adding missing semicolons and removing trailing commas
  • Enhanced task creation query with proper version handling using Common Table Expression
  • Added proper error handling for AssertionError cases returning 404 status
  • Improved docs route tests:
    • Added verification of GET request
    • Fixed content field format
    • Added clear skip reasons for failing FTS-related tests

Changes walkthrough 📝

Relevant files
Error handling
db_exceptions.py
Add AssertionError handling for resource not found             

agents-api/agents_api/common/utils/db_exceptions.py

  • Added AssertionError handling to return 404 HTTP status code for
    resource not found cases
  • +5/-0     
    Enhancement
    create_or_update_task.py
    Improve task creation query with version handling               

    agents-api/agents_api/queries/tasks/create_or_update_task.py

  • Added CTE (Common Table Expression) to handle version and canonical
    name logic
  • Fixed SQL query formatting and comments
  • Added semicolon at the end of SQL query
  • +22/-3   
    Bug fix
    update_task.py
    Fix SQL syntax in task update query                                           

    agents-api/agents_api/queries/tasks/update_task.py

  • Fixed SQL syntax by removing trailing comma
  • Added missing semicolon at the end of SQL query
  • +2/-2     
    Tests
    test_docs_routes.py
    Improve docs route tests and handle failing cases               

    agents-api/tests/test_docs_routes.py

  • Changed content field from list to string in test data
  • Added GET request test to verify document content
  • Updated skip decorators with clear reason for test failures
  • Added proper skip annotation for failing tests related to FTS
  • +15/-8   

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


    Important

    Fixes SQL syntax issues, enhances error handling, and improves test coverage and clarity across multiple files.

    • SQL Query Fixes:
      • Added missing semicolons and removed trailing commas in create_or_update_task.py and update_task.py.
      • Enhanced task creation query with CTE for version handling in create_or_update_task.py.
    • Error Handling:
      • Added AssertionError handling in db_exceptions.py to return 404 for resource not found.
    • Tests:
      • Improved test_docs_routes.py by changing content field from list to string and adding GET request verification.
      • Updated skip reasons for failing tests related to FTS in test_docs_routes.py.
      • Added test for listing a single execution transition in test_task_routes.py.
    • Misc:
      • Introduced new health check router in healthz/router.py.

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

    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 4fe4b6c in 23 seconds

    More details
    • Looked at 155 lines of code in 4 files
    • Skipped 0 files when reviewing.
    • Skipped posting 3 drafted comments based on config settings.
    1. agents-api/agents_api/common/utils/db_exceptions.py:146
    • Draft comment:
      Mapping AssertionError to a 404 status code might not be appropriate in all cases, as assertions can fail for reasons other than resource non-existence. Consider if a different status code might be more suitable.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The addition of the AssertionError mapping to a 404 HTTPException is consistent with the existing pattern of exception handling in this file. However, the choice of a 404 status code for an AssertionError might not always be appropriate, as assertions can fail for various reasons not necessarily related to resource existence.
    2. agents-api/agents_api/queries/tasks/create_or_update_task.py:118
    • Draft comment:
      The workflows_query uses the current version without incrementing it. Consider incrementing the version to avoid overwriting the same version repeatedly.
    • Reason this comment was not posted:
      Comment was not on a valid diff hunk.
    3. agents-api/tests/test_docs_routes.py:56
    • Draft comment:
      The content field was changed from a list to a string. Ensure this change is consistent across all related tests to maintain uniformity.
    • Reason this comment was not posted:
      Confidence changes required: 30%
      The test for creating an agent doc was modified to change the content field from a list to a string. This change should be consistent across all related tests to ensure uniformity.

    Workflow ID: wflow_Y7Y5Wo5OUCL48s3o


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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    SQL Query Logic

    The CTE query logic for version handling should be carefully validated to ensure it correctly handles all edge cases, especially around concurrent updates and version conflicts.

    WITH current_version AS (
        SELECT COALESCE(
            (SELECT MAX("version")
             FROM tasks
             WHERE developer_id = $1
               AND task_id = $4),
            0
        ) + 1 as next_version,
        COALESCE(
            (SELECT canonical_name
             FROM tasks
             WHERE developer_id = $1 AND task_id = $4
             ORDER BY version DESC
             LIMIT 1),
            $2
        ) as effective_canonical_name
        FROM (SELECT 1) as dummy
    )
    Test Coverage

    Multiple tests are being skipped due to FTS issues in the test container. Consider adding alternative test approaches or fixing the underlying FTS functionality in the test environment.

    @skip("Fails due to FTS not working in Test Container")
    @test("route: search agent docs")
    async def _(make_request=make_request, agent=test_agent, doc=test_doc):
        await asyncio.sleep(0.5)
        search_params = {
            "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
    
    @skip("Fails due to FTS not working in Test Container")
    @test("route: search user docs")
    async def _(make_request=make_request, user=test_user, doc=test_user_doc):
        await asyncio.sleep(0.5)
        search_params = {
            "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
    
    
    @skip("Fails due to Vectorizer and FTS not working in Test Container")

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix incorrect CTE reference in SQL query to prevent runtime errors

    The query references a CTE named 'version' in the FROM clause, but this CTE is not
    defined. The query should reference the 'current_version' CTE instead.

    agents-api/agents_api/queries/tasks/create_or_update_task.py [121]

    -FROM version;
    +FROM current_version;
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: The query incorrectly references a non-existent CTE 'version' instead of the defined 'current_version' CTE, which would cause a SQL error at runtime. This is a critical bug fix.

    10
    General
    Replace arbitrary sleep delays with proper asynchronous wait mechanisms to prevent flaky tests

    The asyncio.sleep(0.5) calls in tests are arbitrary delays that could lead to flaky
    tests. Consider implementing a proper wait mechanism or event-based synchronization.

    agents-api/tests/test_docs_routes.py [179-180]

    -await asyncio.sleep(0.5)
    +# Use proper wait mechanism
    +await wait_for_index_update()  # implement this helper
     search_params = {
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using fixed sleep delays can lead to flaky tests. Implementing a proper wait mechanism would make tests more reliable and maintainable.

    7
    Add cleanup steps in test to ensure proper test isolation and prevent test data pollution

    Add a cleanup step after the test to remove the created document, ensuring proper
    test isolation and preventing data leaks between test runs.

    agents-api/tests/test_docs_routes.py [74]

     assert response.json()["content"] == "This is a test agent document."
     
    +# Cleanup
    +cleanup_response = make_request(
    +    method="DELETE",
    +    url=f"/docs/{doc_id}",
    +)
    +assert cleanup_response.status_code == 200
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: While adding cleanup is good practice for test isolation, the PR already includes a DELETE operation after the test (lines 77-80), making this suggestion redundant.

    4

    Copy link
    Contributor

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

    CI Failure Feedback 🧐

    (Checks updated until commit 213807a)

    Action: Test

    Failed stage: [❌]

    Failed test name: test_execution_workflow

    Failure summary:

    The action failed due to multiple issues:

  • A database connection error occurred: "connection was closed in the middle of operation" during the
    execution of workflow tests
  • The error originated in the execute_system activity with message "cannot perform operation: another
    operation is in progress"
  • Additionally, there was a git submodule issue with path 'drafts/cozo' not found in .gitmodules

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    1322:  PASS  test_agent_queries:44 query: create or update agent sql                2%
    1323:  PASS  test_agent_queries:63 query: update agent sql                          2%
    1324:  PASS  test_agent_queries:85 query: get agent not exists sql                  3%
    1325:  PASS  test_agent_queries:96 query: get agent exists sql                      4%
    1326:  PASS  test_agent_queries:111 query: list agents sql                          4%
    1327:  PASS  test_agent_queries:122 query: patch agent sql                          5%
    1328:  PASS  test_agent_queries:143 query: delete agent sql                         5%
    1329:  INFO:httpx:HTTP Request: POST http://testserver/agents "HTTP/1.1 403 Forbidden"
    1330:  PASS  test_agent_routes:9 route: unauthorized should fail                    6%
    ...
    
    1395:  INFO:httpx:HTTP Request: GET http://testserver/users/0676f730-4832-7545-8000-5198299bde5d/docs "HTTP/1.1 200 OK"
    1396:  PASS  test_docs_routes:114 route: list user docs                            27%
    1397:  INFO:httpx:HTTP Request: GET http://testserver/agents/0676f730-4b8a-7c78-8000-42fad3d09492/docs "HTTP/1.1 200 OK"
    1398:  PASS  test_docs_routes:128 route: list agent docs                           27%
    1399:  INFO:httpx:HTTP Request: GET http://testserver/users/0676f730-4ef8-72be-8000-8166198cdfab/docs?metadata_filter=%7B%27test%27%3A%20%27test%27%7D "HTTP/1.1 200 OK"
    1400:  PASS  test_docs_routes:142 route: list user docs with metadata filter       28%
    1401:  INFO:httpx:HTTP Request: GET http://testserver/agents/0676f730-524b-75a7-8000-4761f9f986a5/docs?metadata_filter=%7B%27test%27%3A%20%27test%27%7D "HTTP/1.1 200 OK"
    1402:  PASS  test_docs_routes:159 route: list agent docs with metadata filter      29%
    1403:  SKIP  test_docs_routes:176 route: search agent       Fails due to FTS not   29%
    1404:  docs                        working in Test         
    1405:  Container            
    1406:  SKIP  test_docs_routes:199 route: search user docs   Fails due to FTS not   30%
    1407:  working in Test         
    1408:  Container            
    1409:  SKIP  test_docs_routes:223 route: search agent           Fails due to       30%
    ...
    
    1434:  PASS  test_execution_workflow:29 workflow: evaluate step single             39%
    1435:  PASS  test_execution_workflow:68 workflow: evaluate step multiple           39%
    1436:  PASS  test_execution_workflow:110 workflow: variable access in expressions  40%
    1437:  PASS  test_execution_workflow:152 workflow: yield step                      40%
    1438:  PASS  test_execution_workflow:201 workflow: sleep step                      41%
    1439:  PASS  test_execution_workflow:251 workflow: return step direct              42%
    1440:  PASS  test_execution_workflow:295 workflow: return step nested              42%
    1441:  PASS  test_execution_workflow:346 workflow: log step                        43%
    1442:  ERROR:temporalio.activity:Error in log_step: 'dict object' has no attribute 'hell' ({'activity_id': '1', 'activity_type': 'log_step', 'attempt': 1, 'namespace': 'default', 'task_queue': 'julep-task-queue', 'workflow_id': '2ccf5dca-88da-42cb-bf85-90b48c465a1c', 'workflow_run_id': '70bf4753-1934-48f0-951b-3e59f5c470b0', 'workflow_type': 'TaskExecutionWorkflow'})
    1443:  ERROR:temporalio.workflow:Error in step 1: 'dict object' has no attribute 'hell' ({'attempt': 1, 'namespace': 'default', 'run_id': '70bf4753-1934-48f0-951b-3e59f5c470b0', 'task_queue': 'julep-task-queue', 'workflow_id': '2ccf5dca-88da-42cb-bf85-90b48c465a1c', 'workflow_type': 'TaskExecutionWorkflow'})
    1444:  PASS  test_execution_workflow:396 workflow: log step expression fail        43%
    1445:  ERROR:temporalio.activity:Error in execute_system_call: cannot perform operation: another operation is in progress ({'activity_id': '3', 'activity_type': 'execute_system', 'attempt': 1, 'namespace': 'default', 'task_queue': 'julep-task-queue', 'workflow_id': '0676f732-754d-7ba4-8000-241ed6b229f9', 'workflow_run_id': '114f1f66-6007-4774-afe4-c0bcfe3b2d9b', 'workflow_type': 'TaskExecutionWorkflow'})
    1446:  ERROR:asyncio:Future exception was never retrieved
    1447:  future: <Future finished exception=ConnectionDoesNotExistError('connection was closed in the middle of operation')>
    1448:  asyncpg.exceptions.ConnectionDoesNotExistError: connection was closed in the middle of operation
    1449:  ##[error]The operation was canceled.
    ...
    
    1451:  [command]/usr/bin/git version
    1452:  git version 2.47.1
    1453:  Temporarily overriding HOME='/home/runner/work/_temp/97c895ec-f6c8-444f-a343-e7e58693c611' before making global git config changes
    1454:  Adding repository directory to the temporary git global config as a safe directory
    1455:  [command]/usr/bin/git config --global --add safe.directory /home/runner/work/julep/julep
    1456:  [command]/usr/bin/git config --local --name-only --get-regexp core\.sshCommand
    1457:  [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' || :"
    1458:  fatal: No url found for submodule path 'drafts/cozo' in .gitmodules
    1459:  ##[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.

    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 2fb9970 in 26 seconds

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

    Workflow ID: wflow_9LEZu8YILAHard1W


    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 27e3a08 in 29 seconds

    More details
    • Looked at 49 lines of code in 4 files
    • Skipped 0 files when reviewing.
    • Skipped posting 1 drafted comments based on config settings.
    1. agents-api/agents_api/routers/healthz/router.py:3
    • Draft comment:
      The router variable is defined but not used. Consider removing it to clean up the code.
    • Reason this comment was not posted:
      Decided after close inspection that this draft comment was likely wrong and/or not actionable:
      This is a new file in a FastAPI project, likely part of a health check endpoint setup. The router variable is a standard FastAPI pattern - it's typically defined in a module and then imported elsewhere to be included in the main app. Just because we don't see it used in this file doesn't mean it's unused in the project.
      I might be making assumptions about the FastAPI project structure. Maybe this really is dead code.
      FastAPI's router pattern is so standard and fundamental that this is almost certainly meant to be imported and used elsewhere. The file name "healthz" strongly suggests this is for health check endpoints.
      The comment should be deleted. The router is likely used by importing it from other files, which is a standard FastAPI pattern.

    Workflow ID: wflow_FGYsiLIE7olWWVsk


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

    @creatorrr creatorrr merged commit 81841e4 into f/switch-to-pg Dec 31, 2024
    1 check passed
    @creatorrr creatorrr deleted the x/misc-test-fixes branch December 31, 2024 09:15
    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