-
Notifications
You must be signed in to change notification settings - Fork 48
feat: consume dmv deck.gl bulk annotations; drop clustering UI #405
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
Open
igoroctaviano
wants to merge
12
commits into
master
Choose a base branch
from
feat/bump-dmv-bulk-ann
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
1460b70
feat: consume dmv deck.gl bulk annotations; drop clustering UI
igoroctaviano 575bb42
fix: bump dmv to review-fixed commit; repair allowBuilds entry; guard…
igoroctaviano 2774481
fix: clear measurement on deselection; bump dmv for loading-event and…
igoroctaviano f34667b
ci: allow pnpm build scripts so the git-hosted dmv builds dist/
igoroctaviano 4855dd3
fix: bump dmv so its webpack build succeeds inside pnpm's CI store path
igoroctaviano 298472a
chore: exclude workflow files from SonarCloud analysis
igoroctaviano ac16682
test: add Playwright visual-regression e2e for deck.gl bulk annotations
igoroctaviano 06a9b4f
fix: replace broken serve package with a tiny SPA e2e server
igoroctaviano 250ae70
fix: clear Sonar/DeepSource findings in e2e test infrastructure
igoroctaviano 41dd860
fix: stabilize e2e visual baseline (pin series, overlay-only)
igoroctaviano a29ae56
Merge branch 'master' into feat/bump-dmv-bulk-ann
igoroctaviano 3084620
fix: bump dmv pin to feat/bulk-ann-deckgl tip
igoroctaviano File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| name: slim/e2e-visual-regression | ||
|
|
||
| # Visual-regression tests for the deck.gl bulk-annotation renderer. | ||
| # | ||
| # Runs inside the pinned Playwright container so the (SwiftShader) browser | ||
| # environment is byte-for-byte identical to the committed Linux baselines under | ||
| # e2e/__screenshots__/. See e2e/README.md. | ||
| # | ||
| # The default target study is fetched from the public IDC proxy, so runs depend | ||
| # on that external service; retries (configured in playwright.config.ts) absorb | ||
| # cold-start slowness. This check is intentionally not required for merge — | ||
| # promote it in branch protection once data is hosted locally for hermetic runs. | ||
|
|
||
| on: | ||
| pull_request: | ||
| branches: [master] | ||
| workflow_dispatch: | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: ${{ github.event_name == 'pull_request' }} | ||
|
|
||
| jobs: | ||
| visual: | ||
| name: "Bulk annotation visual regression" | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 45 | ||
| container: | ||
| # Keep this tag in sync with the @playwright/test version in package.json. | ||
| image: mcr.microsoft.com/playwright:v1.62.1-jammy | ||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
|
|
||
| - name: Setup pnpm | ||
| uses: pnpm/action-setup@0ebf47130e4866e96fce0953f49152a61190b271 # v6.0.9 | ||
|
|
||
| - name: Install dependencies | ||
| # Dependency build scripts are gated by allowBuilds in pnpm-workspace.yaml. | ||
| # The git-hosted dicom-microscopy-viewer needs its prepare script to build dist/. | ||
| run: pnpm install --frozen-lockfile | ||
|
|
||
| - name: Build (e2e config) | ||
| run: pnpm run build:e2e | ||
|
|
||
| - name: Run visual-regression tests | ||
| # Playwright starts the static server (serve:e2e) itself via the | ||
| # webServer config and compares against the committed baselines. | ||
| run: pnpm run test:e2e | ||
|
|
||
| - name: Upload Playwright report | ||
| if: ${{ !cancelled() }} | ||
| uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 | ||
| with: | ||
| name: playwright-report | ||
| path: | | ||
| playwright-report/ | ||
| test-results/ | ||
| retention-days: 14 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| # SonarCloud automatic-analysis configuration. | ||
| # | ||
| # Workflow files are excluded because rule githubactions:S6505 flags | ||
| # "pnpm install" without --ignore-scripts. While dicom-microscopy-viewer is | ||
| # pinned to a git commit, its prepare script must run at install time to | ||
| # build dist/. Lifecycle scripts remain gated per-package by the allowBuilds | ||
| # allowlist in pnpm-workspace.yaml, which is stricter than a blanket | ||
| # --ignore-scripts. Remove this exclusion once dmv is pinned to a published | ||
| # npm release again (npm tarballs ship prebuilt dist/). | ||
| # e2e/ and scripts/serve-e2e.mjs are test infrastructure (Playwright + a local | ||
| # static server), not production code. Excluding them keeps the quality gate | ||
| # focused on the shipped app. | ||
| sonar.exclusions=.github/workflows/*.yml,e2e/**,scripts/serve-e2e.mjs,playwright.config.ts |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| # End-to-end / visual-regression tests | ||
|
|
||
| Playwright tests that drive the real app in a real (WebGL-capable) browser and | ||
| compare the deck.gl bulk-annotation overlay against committed screenshot | ||
| baselines, so rendering regressions are caught automatically. | ||
|
|
||
| ## What is covered | ||
|
|
||
| - `bulk-annotations.spec.ts` | ||
| - A large (~396k-polygon) annotation group loads and paints without | ||
| exhausting the JS heap (the OpenLayers Feature pipeline this replaced OOM'd | ||
| on groups this size). | ||
| - The annotation overlay matches a committed screenshot baseline (clipped to | ||
| the map viewport so the memory footer / sidebar never affect the diff). | ||
| - Hiding a group frees the overlay. | ||
|
|
||
| ## Data source | ||
|
|
||
| By default the app is built with `public/config/e2e.js`, which points at the | ||
| **public NCI Imaging Data Commons proxy** — no local DICOM server required. The | ||
| default study is `TCGA-02-0001` (a glioblastoma WSI with a large "Nuclei" | ||
| POLYGON group). | ||
|
|
||
| Override the target without editing code: | ||
|
|
||
| - `E2E_STUDY_UID` — study to open | ||
| - `E2E_SERIES_UID` — SM series to open (pinned so the suite doesn't depend | ||
| on slim's default study→series redirect; this study has four slides) | ||
| - `E2E_GROUP_NAME` — annotation group name to toggle (default `Nuclei`) | ||
| - `E2E_BASE_URL` — point at an already-running server instead of letting | ||
| Playwright serve the build (e.g. a local dcm4chee-backed deployment) | ||
|
|
||
| ### Fully local / hermetic data (optional) | ||
|
|
||
| For a network-independent run you can host the study yourself (e.g. a local | ||
| `dcm4chee` via the | ||
| [imaging-data-commons-skill](https://github.com/ImagingDataCommons/imaging-data-commons-skill) | ||
| to fetch the study, then upload to your DICOMweb server), copy | ||
| `public/config/e2e.js` to point `url` at it, and run the suite normally. This | ||
| avoids proxy rate limits but is heavier to set up and is not used in CI. | ||
|
|
||
| ## Running locally | ||
|
|
||
| ```bash | ||
| # 1. Build once with the e2e config and serve it (or use the dev server: | ||
| # PORT=3977 REACT_APP_CONFIG=e2e pnpm start). | ||
| pnpm run build:e2e && pnpm run serve:e2e # terminal A | ||
| # 2. Run the tests (reuses the running server). | ||
| pnpm run test:e2e # terminal B | ||
| ``` | ||
|
|
||
| `serve:e2e` is a tiny dependency-free SPA static server | ||
| (`scripts/serve-e2e.mjs`) that listens on port **3977** (chosen so it never | ||
| collides with other common `:3000` dev servers). Playwright can also start it | ||
| itself via the `webServer` config when nothing is listening on that port. | ||
|
|
||
| ## Screenshot baselines | ||
|
|
||
| WebGL output is made deterministic across machines by forcing ANGLE + | ||
| SwiftShader (software rendering). SwiftShader still differs between operating | ||
| systems, so **baselines are per-OS** and the ones committed here are **Linux**, | ||
| matching CI. | ||
|
|
||
| Regenerate them with the pinned Playwright container (browser runs in the | ||
| container; the app is served from the host): | ||
|
|
||
| ```bash | ||
| pnpm run build:e2e && pnpm run serve:e2e # terminal A | ||
| pnpm run test:e2e:update:docker # terminal B | ||
| # review the diff under e2e/__screenshots__/ and commit | ||
| ``` | ||
|
|
||
| Generating baselines with a plain `pnpm run test:e2e:update` on macOS/Windows | ||
| produces host-OS snapshots that will **not** match CI — always use the Docker | ||
| script for committed baselines. | ||
|
|
||
| ## CI | ||
|
|
||
| `.github/workflows/e2e-visual.yml` runs this suite inside the same pinned | ||
| Playwright container, so the browser environment is byte-for-byte identical to | ||
| the local baseline-generation path above. |
Binary file added
BIN
+426 KB
e2e/__screenshots__/bulk-annotations.spec.ts/nuclei-whole-slide-linux.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| import { expect, test } from '@playwright/test' | ||
|
|
||
| import { | ||
| GROUP_NAME, | ||
| SERIES_UID, | ||
| STUDY_UID, | ||
| deckDrawnPixelCount, | ||
| expandAnnotationGroups, | ||
| mapClip, | ||
| setGroupVisibility, | ||
| prepareAnnotationScreenshot, | ||
| usedHeapMB, | ||
| waitForAnnotationsCleared, | ||
| waitForAnnotationsDrawn, | ||
| waitForAnnotationsStable, | ||
| waitForSlide, | ||
| } from './helpers' | ||
|
|
||
| /** | ||
| * Visual-regression + smoke coverage for the deck.gl bulk-annotation renderer. | ||
| * | ||
| * The default target is a ~396k-polygon "Nuclei" group (see helpers.ts). The | ||
| * suite asserts three things that matter for the renderer: | ||
| * 1. A large group loads and paints without exhausting the JS heap (the OL | ||
| * Feature pipeline this replaced OOM'd on groups this size). | ||
| * 2. The annotation overlay matches a committed screenshot baseline. | ||
| * 3. Hiding the group frees the overlay. | ||
| * | ||
| * Screenshots are clipped to the map viewport so the volatile memory footer | ||
| * and sidebar text never enter the comparison. | ||
| */ | ||
| test.describe('bulk annotations (deck.gl overlay)', () => { | ||
| test.beforeEach(async ({ page }) => { | ||
| await page.goto(`/studies/${STUDY_UID}/series/${SERIES_UID}`) | ||
| await waitForSlide(page) | ||
| await expandAnnotationGroups(page) | ||
| }) | ||
|
|
||
| test('renders a large annotation group without exhausting memory', async ({ | ||
| page, | ||
| }) => { | ||
| const heapBefore = await usedHeapMB(page) | ||
|
|
||
| await setGroupVisibility(page, GROUP_NAME, true) | ||
|
|
||
| const drawnPixels = await waitForAnnotationsDrawn(page) | ||
| expect(drawnPixels).toBeGreaterThan(500) | ||
|
|
||
| // Wait for progressive streaming to finish so the whole-slide overlay | ||
| // (the deterministic screenshot target) reflects the complete group. | ||
| await waitForAnnotationsStable(page) | ||
|
|
||
| const heapAfter = await usedHeapMB(page) | ||
| if (heapBefore >= 0 && heapAfter >= 0) { | ||
| // Guardrail against a regression to the old multi-GB / OOM behavior. | ||
| // ~396k polygons decode to a few hundred MB; 1500 MB leaves headroom | ||
| // without letting a runaway leak pass. | ||
| expect(heapAfter).toBeLessThan(1500) | ||
| } | ||
|
|
||
| // The whole-slide fit is fully deterministic (image dimensions + viewport). | ||
| // Hide the WSI tile layers / chrome so the screenshot compares only the | ||
| // deck.gl overlay — WSI tile decoding is non-deterministic across runs. | ||
| await page | ||
| .waitForLoadState('networkidle', { timeout: 60_000 }) | ||
| .catch(() => undefined) | ||
| await prepareAnnotationScreenshot(page) | ||
| await expect(page).toHaveScreenshot('nuclei-whole-slide.png', { | ||
| clip: await mapClip(page), | ||
| timeout: 120_000, | ||
| }) | ||
| }) | ||
|
|
||
| test('hides an annotation group on toggle-off', async ({ page }) => { | ||
| await setGroupVisibility(page, GROUP_NAME, true) | ||
| await waitForAnnotationsDrawn(page) | ||
|
|
||
| await setGroupVisibility(page, GROUP_NAME, false) | ||
|
|
||
| await waitForAnnotationsCleared(page) | ||
| expect(await deckDrawnPixelCount(page)).toBeLessThan(500) | ||
| }) | ||
| }) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.