Skip to content

test: limit list operation in CLI test#492

Closed
olivermeyer wants to merge 2 commits intomainfrom
test/limit-list-in-test
Closed

test: limit list operation in CLI test#492
olivermeyer wants to merge 2 commits intomainfrom
test/limit-list-in-test

Conversation

@olivermeyer
Copy link
Collaborator

Limits the number of runs returned by the list operations in test_cli_run_submit_and_describe_and_cancel_and_download_and_delete to avoid executing a heavy query dozens of time per operation.

Copilot AI review requested due to automatic review settings March 18, 2026 10:22
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates an end-to-end CLI test to cap application run list results via --limit, reducing repeated heavy list queries during test_cli_run_submit_and_describe_and_cancel_and_download_and_delete.

Changes:

  • Import LIST_APPLICATION_RUNS_MAX_PAGE_SIZE and reuse it as the --limit value across multiple run list invocations in the complex CLI e2e test.
  • Add --limit to several application run list calls that filter by --query, --note-regex, and --tags.

@olivermeyer olivermeyer force-pushed the test/limit-list-in-test branch from 80110dd to 10eb6dd Compare March 18, 2026 10:27
Copilot AI review requested due to automatic review settings March 18, 2026 10:28
@olivermeyer olivermeyer added the claude Trigger Claude Code automation label Mar 18, 2026
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR optimizes an end-to-end CLI test by adding an explicit --limit to multiple application run list invocations, preventing the underlying list operation from iterating through many pages of results (and thus avoiding repeated heavy backend queries). It also improves the GitHub Actions failure summary labels for the E2E suites.

Changes:

  • Add --limit (using LIST_APPLICATION_RUNS_MAX_PAGE_SIZE) to repeated application run list calls in a long-running CLI E2E test to cap pagination work.
  • Update the “Assert no test failures” step to report more specific E2E suite names in CI output.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
tests/aignostics/application/cli_test.py Adds a --limit argument to multiple run list CLI invocations to avoid enumerating all runs during the test.
.github/workflows/_test.yml Renames the reported failing suite labels for the E2E steps to be more specific (regular/long/very-long).

@claude
Copy link

claude bot commented Mar 18, 2026

✅ PR Review Complete - APPROVED

Reviewer: Claude Code (Medical Device Software Review)
PR: #492 - test: limit list operation in CLI test
Branch: test/limit-list-in-test
Author: @olivermeyer


Summary

This PR makes two focused improvements:

  1. Performance optimization: Limits list operations in a complex E2E test to avoid heavy queries
  2. Improved observability: Clarifies test suite naming in CI/CD failure reporting

Status: ✅ APPROVED - All checks passed, no blocking issues found.


Critical Checks ✅

1. Test Markers ✅

  • Verified test test_cli_run_submit_and_describe_and_cancel_and_download_and_delete has proper markers: @pytest.mark.e2e and @pytest.mark.long_running
  • Ran unmarked test detection: 0 unmarked tests found
    uv run pytest -m "not unit and not integration and not e2e" --collect-only
    # Result: 0 selected (all 37 tests properly marked)

2. Linting ✅

All linting checks passed:

  • ✅ Ruff formatting (165 files already formatted)
  • ✅ Ruff linting (all checks passed)
  • ✅ PyRight type checking (0 errors, 0 warnings)
  • ✅ MyPy strict mode (no issues in 81 source files)

3. Code Quality ✅

  • No new code complexity introduced
  • Changes are minimal and focused
  • Import of LIST_APPLICATION_RUNS_MAX_PAGE_SIZE is appropriate and follows SDK conventions

4. Conventional Commits ✅

Both commits follow conventional commit format:

  • test: limit list operation in CLI test
  • chore: update test suite name when reporting failures

5. Test Coverage ✅

  • No new code to cover
  • Changes are test-only improvements
  • Coverage requirements not affected

Detailed Analysis

Change 1: Test Performance Optimization (cli_test.py)

What changed: Added --limit LIST_APPLICATION_RUNS_MAX_PAGE_SIZE to 11 list operations in the test.

Why this matters:

  • The test test_cli_run_submit_and_describe_and_cancel_and_download_and_delete is marked as @pytest.mark.long_running with a 10-minute timeout
  • This test exercises query/filter/tag functionality by calling list operations multiple times
  • Without limits, queries could return hundreds/thousands of runs from staging, causing:
    • Unnecessary API load
    • Slower test execution
    • Potential timeout issues

Impact:

  • ✅ Reduces query load on staging environment
  • ✅ Faster test execution
  • ✅ No loss of test coverage (test verifies filtering logic, not pagination limits)
  • ✅ Uses the SDK's own constant (100 runs), ensuring consistency

Change 2: Improved CI/CD Observability (_test.yml)

What changed: Updated test suite failure reporting names:

  • "e2e""e2e-regular"
  • "long-running""e2e-long-running"
  • "very-long-running""e2e-very-long-running"

Why this matters:

  • Makes it immediately clear that long-running tests are a subcategory of E2E tests
  • Aligns with test categorization documented in .github/CLAUDE.md
  • Improves debugging when CI fails by showing exactly which E2E subset failed

Impact:

  • ✅ Better observability for developers and reviewers
  • ✅ Aligns naming with SDK documentation
  • ✅ No functional change to CI logic

Architecture & Medical Device Compliance ✅

Modulith Architecture Compliance ✅

  • No module boundary violations
  • Test changes are isolated to application module tests
  • Import from aignostics.platform is appropriate (cross-module constant)

Medical Device & Security ✅

  • No secrets or sensitive data introduced
  • No changes to production code paths
  • Test-only improvements with no patient data impact
  • Maintains HIPAA compliance posture

CI/CD Impact ✅

  • Workflow syntax is correct
  • No breaking changes to test execution logic
  • BetterStack heartbeats not affected
  • Test marker filtering logic unchanged

Recommendations

Suggestions (Non-Blocking)

None - This PR is well-scoped and implements exactly what's needed.

Optional Future Enhancements

Consider for future work (not for this PR):

  • Could add a constant for "test default limit" to avoid magic numbers in tests
  • Could document the performance improvement in a commit message (current message is concise but accurate)

Verification Commands

To verify this PR locally:

# 1. Check linting
make lint

# 2. Verify test markers
uv run pytest -m "not unit and not integration and not e2e" --collect-only

# 3. Run the specific test (requires staging credentials)
uv run pytest tests/aignostics/application/cli_test.py::test_cli_run_submit_and_describe_and_cancel_and_download_and_delete -v

# 4. Check all application module tests
make test_e2e -k application

Final Verdict

✅ APPROVED

This is a high-quality, focused PR that:

  • Solves a real performance issue (heavy queries in E2E tests)
  • Improves CI/CD observability
  • Follows all repository conventions
  • Maintains medical device software standards
  • Passes all quality gates

No blocking issues found. Ready to merge after CI passes.


Reviewed with: CLAUDE.md (root), .github/CLAUDE.md, Medical Device Software Standards
CI Pipeline: All checks expected to pass
Risk Level: Low (test-only changes, no production impact)

Great work, @olivermeyer! 🎉

@codecov
Copy link

codecov bot commented Mar 18, 2026

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
727 1 726 15
View the top 1 failed test(s) by shortest run time
tests.aignostics.application.cli_test::test_cli_run_submit_and_describe_and_cancel_and_download_and_delete
Stack Traces | 28s run time
runner = <typer.testing.CliRunner object at 0x7fa1c1b5bcb0>
tmp_path = PosixPath('.../pytest-20/popen-gw3/test_cli_run_submit_and_descri0')
silent_logging = None
record_property = <function record_property.<locals>.append_property at 0x7fa1c1a73a00>

    @pytest.mark.e2e
    @pytest.mark.long_running
    @pytest.mark.flaky(retries=3, delay=5)
    @pytest.mark.timeout(timeout=60 * 10)
    def test_cli_run_submit_and_describe_and_cancel_and_download_and_delete(  # noqa: PLR0915
        runner: CliRunner, tmp_path: Path, silent_logging, record_property
    ) -> None:
        """Check run submit command runs successfully."""
        record_property("tested-item-id", "TC-APPLICATION-CLI-02")
        with submitted_run(
            runner,
            tmp_path,
            CSV_CONTENT_SPOT0,
            extra_args=[
                "--note",
                "note_of_this_complex_test",
                "--tags",
                "cli-test,test_cli_run_submit_and_describe_and_cancel_and_download_and_delete,further-tag",
                "--deadline",
                (datetime.now(tz=UTC) + timedelta(minutes=10)).isoformat(),
                "--onboard-to-aignostics-portal",
                "--gpu-type",
                PIPELINE_GPU_TYPE,
                "--force",
            ],
        ) as run_id:
            # Test that we can find this run by it's note via the query parameter
            list_result = runner.invoke(
                cli,
                [
                    "application",
                    "run",
                    "list",
                    "--query",
                    "note_of_this_complex_test",
                    "--limit",
                    str(LIST_APPLICATION_RUNS_MAX_PAGE_SIZE),
                ],
            )
            assert list_result.exit_code == 0
            list_output = normalize_output(list_result.stdout)
            assert run_id in list_output, f"Run ID '{run_id}' not found when filtering by note via query"
    
            # Test that we can find this run by it's tag via the query parameter
            list_result = runner.invoke(
                cli,
                [
                    "application",
                    "run",
                    "list",
                    "--query",
                    "test_cli_run_submit_and_describe_and_cancel_and_download_and_delete",
                    "--limit",
                    str(LIST_APPLICATION_RUNS_MAX_PAGE_SIZE),
                ],
            )
            assert list_result.exit_code == 0
            list_output = normalize_output(list_result.stdout)
>           assert run_id in list_output, f"Run ID '{run_id}' not found when filtering by tag via query"
E           AssertionError: Run ID 'cfbd5ded-93ad-4fef-a04f-67ffdcb755f8' not found when filtering by tag via query
E           assert 'cfbd5ded-93ad-4fef-a04f-67ffdcb755f8' in "Application Run IDs:- a3c92877-7ade-44de-af68-9fc8059feeec of he-tme (1.0.0-beta.8) (submitted: 2025-10-26 12:18:46 UTC, status: TERMINATED (RunTerminationReason.CANCELED_BY_USER) | error: The run was cancelled by the user. Query .../runs/a3c92877-7ade-44de-af68-9fc8059feeec/items to get the items which where successfully processed before cancellation. (N/A), output: NONE)- c98f6d68-2ee1-4727-b664-adabcece5345 of he-tme (1.0.0-beta.8) (submitted: 2025-10-25 17:23:45 UTC, status: TERMINATED (RunTerminationReason.CANCELED_BY_USER) | error: The run was cancelled by the user. Query .../runs/c98f6d68-2ee1-4727-b664-adabcece5345/items to get the items which where successfully processed before cancellation. (N/A), output: NONE)- 424a39b1-d231-4f3f-9920-352109e05898 of he-tme (1.0.0-beta.8) (submitted: 2025-10-25 17:02:42 UTC, status: TERMINATED (RunTerminationReason.CANCELED_BY_USER) | error: The run was cancelled by the user. Query .../runs/424a39b1-d231-4f3f-9920-352109e05898/items to get the items which where successfully processed before cancellation. (N/A), output: NONE)- a8fe6625-fdf6-4e5b-ac39-2db67fd8a27b of he-tme (1.0.0-beta.8) (submitted: 2025-10-25 12:18:23 UTC, status: TERMINATED (...4697-9582-53af05ea72eb/items to get the items which where successfully processed before cancellation. (N/A), output: NONE)- 91218f5b-40b6-4d37-88cc-2f85779d5c7c of he-tme (1.0.0-beta.8) (submitted: 2025-10-21 20:28:05 UTC, status: TERMINATED (RunTerminationReason.CANCELED_BY_USER) | error: The run was cancelled by the user. Query .../runs/91218f5b-40b6-4d37-88cc-2f85779d5c7c/items to get the items which where successfully processed before cancellation. (N/A), output: NONE)- 03d78684-872b-4959-877a-d664d521009d of he-tme (1.0.0-beta.8) (submitted: 2025-10-21 20:23:34 UTC, status: TERMINATED (RunTerminationReason.CANCELED_BY_USER) | error: The run was cancelled by the user. Query .../runs/03d78684-872b-4959-877a-d664d521009d/items to get the items which where successfully processed before cancellation. (N/A), output: NONE)- 9f10885f-b967-48c4-a365-db6b42051ec6 of he-tme (1.0.0-beta.8) (submitted: 2025-10-21 20:23:09 UTC, status: TERMINATED (RunTerminationReason.CANCELED_BY_USER) | error: The run was cancelled by the user. Query .../runs/9f10885f-b967-48c4-a365-db6b42051ec6/items to get the items which where successfully processed before cancellation. (N/A), output: NONE)Listed '100' run(s)."

.../aignostics/application/cli_test.py:543: AssertionError

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@sonarqubecloud
Copy link

@olivermeyer olivermeyer marked this pull request as draft March 18, 2026 16:37
@olivermeyer
Copy link
Collaborator Author

Fixing in #495

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

Labels

claude Trigger Claude Code automation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants