test: mock BucketService.find_static in GUI test

[olivermeyer]

test_gui_bucket_shows sometimes exceeds the 10s pytest timeout. Most of that is spent in the teardown phase (observed up to 9s). The hypothesis is that user.open("/bucket") calls _get_rows() which runs Service.find_static in a thread; the test immediately succeeds because the test user sees the expected message, but listing the files in the bucket is still running in the background. The teardown lasts until this listing operation ends, which sometimes pushes it over the 10s timeout. I fix this by patching the find_static method to return an empty list.

Observed teardown time before: 8-9s
Observed teardown time after: 2-3s

[Copilot]

Pull request overview

This PR mocks the BucketService.find_static method in the test_gui_bucket_shows integration test to prevent test timeouts caused by the slow I/O-bound operation of listing cloud bucket objects.

Changes:

• Patches Service.find_static with an empty list return value in the bucket GUI integration test to avoid slow cloud storage calls during test execution.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.
see 8 files with indirect coverage changes

[claude]

Claude finished @olivermeyer's task in 2m 40s —— View job

---

PR Review: Mock BucketService.find_static in GUI test

Summary

✅ APPROVED - This is an excellent fix that correctly addresses a timeout issue in an integration test by mocking an I/O-bound cloud storage operation. The approach is appropriate, follows repository conventions, and demonstrates best practices for integration testing.

---

Todo List

• Read repository documentation (CLAUDE.md, .github/CLAUDE.md)
• Read changed test file
• Verify test markers and categorization
• Assess mocking approach appropriateness
• Check for similar issues in other tests
• Provide comprehensive review feedback

---

Review Findings

✅ Correct Test Categorization

The test test_gui_bucket_shows is correctly marked as @pytest.mark.integration:

@pytest.mark.integration
async def test_gui_bucket_shows(user: User) -> None:

This is appropriate because:

• ✅ Tests GUI→Service interaction (presentation layer → service layer)
• ✅ Should pass offline (external services mocked)
• ✅ Must stay within 10s default timeout
• ✅ Follows repository's testing strategy (see CLAUDE.md line 743-748)

✅ Appropriate Mocking Strategy

What's being mocked:

with patch("aignostics.bucket._gui.Service.find_static", return_value=[]):
    await user.open("/bucket")
    await user.should_see("The bucket is securely hosted on Google Cloud in EU")

Why this is correct:

1. Service.find_static() is I/O-bound: Lines 462-478 in _service.py show this method:

   • Makes S3 API calls via boto3 paginator
   • Lists all objects in GCS bucket (can be hundreds/thousands of files)
   • Called in background thread by GUI (run.io_bound() on line 76-79 of _gui.py)

2. Teardown timing issue confirmed: The test succeeds immediately after seeing the expected text, but the background thread continues listing bucket objects. NiceGUI test teardown waits for all threads to complete, causing the 8-9s delay.

3. Mock target is correct: Mocked at the GUI module's import path (aignostics.bucket._gui.Service.find_static), not the service module. This is the right approach per unittest.mock documentation.

4. Empty list return is appropriate: The test validates page rendering, not bucket contents. Returning [] simulates an empty bucket, which is a valid state.

✅ Follows Repository Patterns

This fix mirrors the pattern in tests/aignostics/dataset/gui_test.py:15-19:

@pytest.mark.integration
async def test_gui_idc_shows(user: User) -> None:
    """Test that the user sees the dataset page."""
    await user.open("/dataset/idc")
    await user.should_see("Explore Portal")

Both tests validate page rendering, not business logic. The bucket test now correctly mocks the slow I/O operation.

✅ Performance Improvement Validated

Before fix:

• Teardown: 8-9 seconds
• Frequent timeout failures (>10s)

After fix (verified):

test teardown duration top
total          name                                                       num med            min           
0:00:02.446674 tests/aignostics/bucket/gui_test.py::test_gui_bucket_shows   1 0:00:02.446674 0:00:02.446674

• Teardown: 2.4 seconds ✅
• Well within 10s timeout
• 73% reduction in teardown time

✅ Clean Code Quality

• Proper import: from unittest.mock import patch
• Clear comment explaining why: # Service.find_static can be slow and lead to test timeouts
• Context manager scope is minimal (only wraps the GUI interaction)
• No side effects on other tests

---

Suggestions (Non-Blocking)

1. Consider Adding Timeout Override (Optional)

While the fix works, you could be extra defensive:

@pytest.mark.integration
@pytest.mark.timeout(timeout=20)  # Optional: Give extra buffer for NiceGUI teardown
async def test_gui_bucket_shows(user: User) -> None:

Rationale: NiceGUI teardown can vary across environments. A 20s timeout is still fast but provides buffer.

2. Document This Pattern (Optional)

Consider adding to tests/CLAUDE.md or tests/aignostics/bucket/CLAUDE.md:

### GUI Integration Test Pattern

When testing GUI page rendering that triggers I/O-bound service operations:
1. Mock the service method at the GUI import path
2. Return minimal valid data (empty list for collections)
3. Test only page rendering, not business logic

Example: test_gui_bucket_shows mocks Service.find_static to avoid 
cloud storage listing during teardown.

---

Other CI Failures (Unrelated to This PR)

The 2 failing tests are NOT caused by this change:

1. test_gui_run_download - File size assertion failure for tissue_qc_geojson_polygons.json
2. test_gui_run_qupath_install_to_inspect - Same file size assertion

Root cause: The expected file size (160668 bytes ±10%) doesn't match actual (259955 bytes). This is:

• ✅ Unrelated to bucket mocking
• ⚠️ Suggests the HETA application version changed output format
• 💡 Fix needed: Update SPOT_0_EXPECTED_RESULT_FILES constant in tests/constants_test.py

Note: There's already a fix merged for this (commit 7dfaf52 in this PR's branch merge).

---

Medical Device Compliance ✅

• No security concerns: Mocking local method, no credentials exposed
• Data privacy maintained: No real bucket data accessed in test
• Deterministic behavior: Empty list mock ensures consistent test behavior

---

Final Recommendation

✅ APPROVE AND MERGE

This PR:

• ✅ Correctly fixes the timeout issue
• ✅ Follows repository testing conventions
• ✅ Improves test reliability and execution time
• ✅ Has appropriate test marker (@pytest.mark.integration)
• ✅ Uses proper mocking technique
• ✅ No breaking changes
• ✅ Clean, well-commented code

The fix demonstrates excellent understanding of:

• Integration vs E2E test boundaries
• NiceGUI async test lifecycle
• Python unittest.mock best practices
• Repository testing standards

Great work! 🎉

---

[Copilot]

Pull request overview

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

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud