Skip to content

Conversation

@helmut-hoffer-von-ankershoffen
Copy link
Contributor

Fixes #211

Added missing expires_seconds argument to _get_three_spots_payload_for_test() call in test_platform_test_app_submit_and_wait test. The function requires 1 positional argument but was being called without any.

Changes

  • Updated line 302 in tests/aignostics/platform/e2e_test.py
  • Added expires_seconds=TEST_APPLICATION_SUBMIT_AND_FIND_DEADLINE_SECONDS + 60 * 5
  • Consistent with pattern used in other tests in the same file

Generated with Claude Code

…_for_test

Add missing expires_seconds argument to _get_three_spots_payload_for_test() call in test_platform_test_app_submit_and_wait test. The function requires 1 positional argument but was being called without any.

Fixes #211

Co-authored-by: Helmut Hoffer von Ankershoffen né Oertel <helmut-hoffer-von-ankershoffen@users.noreply.github.com>
@helmut-hoffer-von-ankershoffen helmut-hoffer-von-ankershoffen merged commit d62a27f into main Oct 27, 2025
11 checks passed
@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2025

Claude finished @helmut-hoffer-von-ankershoffen's task —— View job


✅ PR Review Complete

PR #213: Fix for missing expires_seconds argument in test


Summary

This PR correctly fixes a critical bug in test_platform_test_app_submit_and_wait() where the helper function _get_three_spots_payload_for_test() was called without its required positional argument. The fix is correct, consistent with the codebase pattern, and follows all repository standards.


✅ What's Good

1. Correct Fix

The function signature requires expires_seconds:

def _get_three_spots_payload_for_test(expires_seconds: int) -> list[platform.InputItem]:

The fix at line 302-304 now correctly passes the argument:

payload=_get_three_spots_payload_for_test(
    expires_seconds=TEST_APPLICATION_SUBMIT_AND_FIND_DEADLINE_SECONDS + 60 * 5
)

2. Consistent Pattern

The fix follows the exact same pattern used in:

  • Line 353-355 in test_platform_test_app_submit()
  • Line 379-381 in test_platform_test_app_find()
  • All calls to _get_single_spot_payload_for_heta() (lines 329, 403, 429)

The formula DEADLINE_SECONDS + 60 * 5 ensures signed URLs expire 5 minutes after the test deadline, which is the correct approach.

3. Test Markers Present

The test has all required markers:

@pytest.mark.skip(reason="v0.0.4 on production balking...")
@pytest.mark.e2e
@pytest.mark.long_running
@pytest.mark.timeout(timeout=TEST_APPLICATION_SUBMIT_AND_WAIT_DEADLINE_SECONDS + 60 * 5)

Critical: The @pytest.mark.e2e marker is present, ensuring this test runs in CI (per CLAUDE.md requirements).

4. Conventional Commit Format

fix: add missing expires_seconds argument to _get_three_spots_payload_for_test (#213)
  • Type: fix ✅ (correct for bug fix)
  • Scope: None (acceptable for test-only changes)
  • Description: Clear and accurate

5. Minimal Change

The PR changes only what's necessary:

  • 1 file modified: tests/aignostics/platform/e2e_test.py
  • 3 lines added, 1 deleted: Surgical fix
  • No unnecessary refactoring or scope creep

📋 Review Checklist Results

Check Status Notes
Test Markers ✅ Pass Has required @pytest.mark.e2e marker
Test Coverage ✅ N/A Test-only change, no coverage impact
Code Quality ✅ Pass Follows repository patterns
Conventional Commits ✅ Pass Proper fix: type
Architecture ✅ Pass Test module, no architecture concerns
Security ✅ Pass No security implications
Breaking Changes ✅ None Internal test fix only
Documentation ✅ N/A No user-facing changes

🎯 Technical Analysis

Issue Root Cause

The test was previously calling:

payload=_get_three_spots_payload_for_test()  # ❌ Missing required argument

This would have caused a TypeError at runtime:

TypeError: _get_three_spots_payload_for_test() missing 1 required positional argument: 'expires_seconds'

Why It Matters

The expires_seconds parameter is critical because:

  1. It controls how long the signed GCS URLs remain valid
  2. Must exceed the test deadline to prevent URL expiration during test execution
  3. The formula DEADLINE + 60 * 5 provides a 5-minute safety buffer

Without this parameter, the test would fail immediately when executed.

Test Status

Note that this test is currently skipped with:

@pytest.mark.skip(reason="v0.0.4 on production balking on whole_slide_image input...")

However, fixing the bug now means:

  1. The test is ready when the underlying issue is resolved
  2. Prevents future confusion when the skip is removed
  3. Maintains consistency across all test functions

🔍 Additional Observations

Positive Patterns Found

  1. Consistent Timeout Strategy

    • All e2e tests use: timeout = DEADLINE_SECONDS + 60 * 5
    • Provides 5-minute buffer for test teardown
    • Applied consistently across 6 test functions
  2. Consistent Expiry Strategy

    • All signed URLs expire: DEADLINE_SECONDS + 60 * 5
    • Ensures URLs outlive the test execution window
    • Same pattern in both _get_three_spots_payload_for_test() and _get_single_spot_payload_for_heta() calls
  3. Test Categorization

    • Proper use of @pytest.mark.e2e and @pytest.mark.long_running
    • Appropriate use of @pytest.mark.scheduled_only for production tests
    • Follows .github/CLAUDE.md test categorization guidelines

✅ Final Verdict

APPROVED

This is a high-quality, necessary bug fix that:

  • ✅ Solves a critical test failure
  • ✅ Follows established patterns perfectly
  • ✅ Maintains code consistency
  • ✅ Adheres to all repository standards
  • ✅ Has proper test markers for CI execution
  • ✅ Uses conventional commit format

No changes requested. The fix is correct and ready to merge.


📚 References

  • Root CLAUDE.md: Test markers and conventional commits
  • .github/CLAUDE.md: E2E test execution strategy
  • tests/CLAUDE.md: Test suite documentation
  • Issue Fix test #211: Original bug report

Reviewed by: Claude Code
Review Date: 2025-10-27
Commit: d62a27f
| Branch: claude/issue-211-20251027-0603

@codecov
Copy link

codecov bot commented Oct 27, 2025

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
557 1 556 8
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 | 49.1s run time
runner = <typer.testing.CliRunner object at 0x7f8ce669c440>
tmp_path = PosixPath('.../pytest-12/popen-gw1/test_cli_run_submit_and_descri0')
silent_logging = None

    @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
    ) -> None:
        """Check run submit command runs successfully."""
        csv_content = "external_id;checksum_base64_crc32c;resolution_mpp;width_px;height_px;staining_method;tissue;disease;"
        csv_content += "platform_bucket_url\n"
        csv_content += ";5onqtA==;0.26268186053789266;7447;7196;H&E;LUNG;LUNG_CANCER;gs:.../bucket/test"
        csv_path = tmp_path / "dummy.csv"
        csv_path.write_text(csv_content)
    
        result = runner.invoke(
            cli,
            [
                "application",
                "run",
                "submit",
                HETA_APPLICATION_ID,
                str(csv_path),
                "--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(),
                "--validate-only",
            ],
        )
        output = normalize_output(result.stdout)
        assert re.search(
            r"Submitted run with id '[0-9a-f-]+' for '",
            output,
        ), f"Output '{output}' doesn't match expected pattern"
        assert result.exit_code == 0
    
        # Extract run ID from the output
        run_id_match = re.search(r"Submitted run with id '([0-9a-f-]+)' for '", output)
        assert run_id_match, f"Failed to extract run ID from output '{output}'"
        run_id = run_id_match.group(1)
    
        # 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",
            ],
        )
        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"
E       AssertionError: Run ID '2c85a2c8-7397-4c9c-84ff-5736bf3c059a' not found when filtering by note via query
E       assert '2c85a2c8-7397-4c9c-84ff-5736bf3c059a' in "Error: Failed to list runs: Failed to retrieve application runs: (500)Reason: Internal Server ErrorHTTP response headers: HTTPHeaderDict({'date': 'Mon, 27 Oct 2025 06:24:34 GMT', 'server': 'envoy', 'content-length': '21', 'content-type': 'text/plain; charset=utf-8', 'x-trace-id': '0a5dbe9b80764cd9b628016088b74a3b', 'x-envoy-upstream-service-time': '5015'})HTTP response body: Internal Server Error"

.../aignostics/application/cli_test.py:300: 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

helmut-hoffer-von-ankershoffen added a commit that referenced this pull request Nov 2, 2025
…_for_test (#213)

Add missing expires_seconds argument to _get_three_spots_payload_for_test() call in test_platform_test_app_submit_and_wait test. The function requires 1 positional argument but was being called without any.

Fixes #211

Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Co-authored-by: Helmut Hoffer von Ankershoffen né Oertel <helmut-hoffer-von-ankershoffen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix test

2 participants