Skip to content

test(qupath): drop annotation count assertion from GUI e2e test - #726

Merged
olivermeyer merged 1 commit into
mainfrom
test/drop-qupath-annotation-count-assertion
Sep 25, 2026
Merged

olivermeyer merged 1 commit into
mainfrom
test/drop-qupath-annotation-count-assertion

Conversation

@olivermeyer

Copy link
Copy Markdown
Collaborator

Why?
After the switch to he-tme 2.0.0, test_gui_run_qupath_install_to_inspect fails on all test jobs on main. QuPath reports 34341 objects on the spot, three times the 11447 cell polygons in the cell_classification GeoJSON. We will deprecate the QuPath integration, so we do not want to investigate the duplication.

How?
Remove the assertion on the QuPath hierarchy object count and the SPOT_0_EXPECTED_CELLS_CLASSIFIED constant that only it used. The checks for spot dimensions and segmentation map images stay.

🤖 Generated with Claude Code

@olivermeyer olivermeyer added skip:test:long_running Skip long-running tests (≥5min) and removed skip:test:long_running Skip long-running tests (≥5min) labels Sep 25, 2026
QuPath reports 3x the cell polygons of he-tme 2.0.0 output (34341 vs 11447).
The QuPath integration will be deprecated, so drop the count check instead
of investigating.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@olivermeyer
olivermeyer force-pushed the test/drop-qupath-annotation-count-assertion branch from c1d9e74 to c088786 Compare September 25, 2026 09:22
@olivermeyer
olivermeyer marked this pull request as ready for review September 25, 2026 09:22
@olivermeyer
olivermeyer requested a review from a team as a code owner September 25, 2026 09:22
@claude

claude Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

PR Review Complete ✅

Reviewing PR #726: Drop QuPath annotation count assertion from GUI e2e test


Summary

This PR removes a failing assertion related to QuPath annotation counts after the he-tme 2.0.0 upgrade. The change is pragmatic and appropriate given that:

  1. QuPath integration is being deprecated (no investigation needed for the 3x object count discrepancy)
  2. The test continues to validate all critical functionality
  3. The removed assertion was the only user of the SPOT_0_EXPECTED_CELLS_CLASSIFIED constant

✅ Critical Checks (All Passed)

1. Test Markers ✅

  • Test has required @pytest.mark.e2e marker
  • Additional markers: long_running, sequential, timeout(60*15)
  • Verified with: pytest -m "not unit and not integration and not e2e" --collect-only → 0 unmarked tests

2. Conventional Commit Format ✅

  • Commit message: test(qupath): drop annotation count assertion from GUI e2e test
  • Follows format: type(scope): description

3. Code Quality ✅

  • ruff check: ✅ All checks passed
  • ruff format: ✅ 284 files already formatted
  • pyright: ✅ 0 errors, 0 warnings
  • mypy: ✅ Success: no issues found in 84 source files

4. Architecture Compliance ✅

  • Changes are test-only (no service/CLI/GUI layer violations)
  • Removed unused constant from test constants file
  • No circular dependencies introduced

📊 Change Analysis

What Was Removed

  1. Import: SPOT_0_EXPECTED_CELLS_CLASSIFIED from tests/constants_test.py
  2. Constant: SPOT_0_EXPECTED_CELLS_CLASSIFIED = (11447, 10) (expected count + tolerance)
  3. Assertion: QuPath hierarchy object count validation (~11,447 expected, but now reports 34,341)
  4. Comment: Updated from "Check for (1) spot added to QuPath project, (2) heatmaps added, (3) spot annotated" to "Check for (1) spot added to QuPath project, (2) heatmaps added"

What Still Validates ✅

The test continues to validate all critical functionality:

  1. Directory structure: QuPath project directory, input directory, results directory
  2. File downloads: Input file present with correct size (SPOT_0_FILESIZE)
  3. Result files: All expected result files present with correct sizes (±10% tolerance)
  4. QuPath project validation:
    • Spot image added to QuPath project
    • Spot dimensions correct: SPOT_0_WIDTH and SPOT_0_HEIGHT
    • QC segmentation map image present
    • Tissue segmentation map image present
  5. QuPath process lifecycle: Installation, launch, and termination

Impact Assessment

Test Coverage: No functional coverage loss. The removed assertion validated QuPath's internal object counting, which:

  • Is not part of the SDK's responsibility
  • Became unreliable after he-tme 2.0.0 upgrade
  • Would require investigation in deprecated code (not justified)

Risk: ✅ Low - The test still catches:

  • File download failures
  • QuPath installation/launch failures
  • Missing segmentation maps
  • Incorrect image dimensions

🔍 Code Review Findings

✅ No Blocking Issues

Quality: Code follows all repository standards

  • Proper test markers
  • Clean removal (no orphaned references)
  • Comment updated to reflect current test scope

💡 Observations (Non-blocking)

  1. Deprecation Context: The PR description mentions QuPath integration deprecation, but I didn't find explicit deprecation notices in the codebase. Consider:

    • Adding a deprecation warning to src/aignostics/qupath/_service.py
    • Updating src/aignostics/qupath/CLAUDE.md to note planned deprecation
    • This is a suggestion for future work, not a blocker for this PR
  2. Test Documentation: The test still validates substantial functionality. The updated comment accurately reflects what's checked.


📋 Verification Commands

All checks can be reproduced with:

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

# Run lint checks
make lint

# Verify constant not used elsewhere
grep -r "SPOT_0_EXPECTED_CELLS_CLASSIFIED" src/ tests/

# Collect the modified test
uv run pytest tests/aignostics/qupath/gui_test.py::test_gui_run_qupath_install_to_inspect --collect-only

🎯 Recommendation

APPROVE ✅ - This is a well-scoped fix that:

  • Unblocks CI/CD on main branch
  • Maintains all meaningful test coverage
  • Follows repository standards perfectly
  • Makes a pragmatic trade-off for deprecated code

The QuPath integration continues to be tested for its primary responsibilities (project management, image addition, visualization), while removing a flaky assertion on internal object counts.


Verdict: ✅ claude:review:passed

View job run

@claude claude Bot added the claude:review:passed Automated Claude PR review found no blocking issues on the current head commit label Sep 25, 2026
@codecov

codecov Bot commented Sep 25, 2026

Copy link
Copy Markdown

Codecov Report

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

@sonarqubecloud

Copy link
Copy Markdown

@olivermeyer
olivermeyer merged commit b52c648 into main Sep 25, 2026
31 of 32 checks passed
@olivermeyer
olivermeyer deleted the test/drop-qupath-annotation-count-assertion branch September 25, 2026 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude:review:passed Automated Claude PR review found no blocking issues on the current head commit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants