docs(testing): align guides with the current Playwright workflow#5941
Open
haoyu-haoyu wants to merge 2 commits into
Open
docs(testing): align guides with the current Playwright workflow#5941haoyu-haoyu wants to merge 2 commits into
haoyu-haoyu wants to merge 2 commits into
Conversation
Update base image from node:20.18.1-slim to node:20.19.0-slim across all Dockerfiles. lerna@9.0.4 requires Node ^20.19.0, so the previous version caused yarn install to fail during Docker builds. Fixes OHIF#5860 Signed-off-by: haoyu-haoyu <haoyu-haoyu@users.noreply.github.com>
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
|
||
|  | ||
| ```bash | ||
| npm install -g dicom10-to-dicomweb |
Contributor
There was a problem hiding this comment.
The PR changed dicomp10-to-dicomweb (the real package on npm/GitHub at chafey/dicomp10-to-dicomweb-js) to dicom10-to-dicomweb, which does not exist on the npm registry. Running this command will fail with a "package not found" error. The original spelling in the file being replaced was correct.
Suggested change
| npm install -g dicom10-to-dicomweb | |
| npm install -g dicomp10-to-dicomweb |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Context
The testing docs had drifted from the way the repo is actually wired today. The main testing page still pointed contributors toward Cypress, while the repository-level end-to-end workflow now runs through Playwright. The root README also had a couple of small inconsistencies that make first-time navigation harder than it needs to be.
Changes & Results
I updated the main testing guide so it points contributors to the current Playwright workflow, links to the dedicated Playwright page, and frames the older package-local Cypress commands as legacy rather than the default path.
I also refreshed the Playwright guide so the commands, report viewer, screenshot notes, and manual server instructions match the current config in the repo.
Finally, I cleaned up the root README by removing a duplicated newsletter block, fixing the community link, and replacing the outdated repository tree with a current overview of the main packages and test directories.
Testing
I ran
git diff --check.I also tried to run a prettier check on the edited files, but the local npm cache in
~/.npmhas a permissions problem on this machine, so that validation did not complete.Checklist
PR
Code
Public Documentation Updates
Tested Environment
Greptile Summary
This PR aligns OHIF's testing documentation with the current Playwright-based workflow, refreshes the root README, and bumps the Node base image to
20.19.0-slimacross all Dockerfiles. The documentation changes are broadly accurate and well-structured, but one typo was introduced intesting.mdthat would cause the test-data install command to fail.Key changes:
testing.md: Replaces the Cypress-centric e2e instructions with a concise Playwright quick-start; frames legacy Cypress commands correctly as package-local/legacy.playwright-testing.md: Switches all commands frombuntoyarn/npx, narrows the screenshot path description to match the currentchromium-only default inplaywright.config.ts, and adds an accurate manual server snippet usingOHIF_PORT=3335.README.md: Removes a duplicated newsletter block, fixes the barecommunity.ohif.orghref tohttps://community.ohif.org, and replaces a stale (and partly duplicated) directory tree with a clean overview that matches the actual repo layout.20.18.1-slim→20.19.0-slim.testing.mdline 82 changed the DICOM conversion tool install command fromdicomp10-to-dicomweb(the real npm package) todicom10-to-dicomweb(which does not exist on the npm registry). This would cause the command to fail for any contributor trying to add new test data.Confidence Score: 4/5
Safe to merge after fixing the
dicom10-to-dicomweb→dicomp10-to-dicomwebtypo intesting.md.All changes are documentation and Docker image bump — no runtime code is touched. The only issue is a single incorrect npm package name in the test-data section of
testing.mdthat would break thenpm installcommand for contributors adding new test data. Everything else cross-checks cleanly againstplaywright.config.tsandpackage.json.platform/docs/docs/development/testing.md— contains the incorrectdicom10-to-dicomwebpackage name at line 82.Important Files Changed
dicom10-to-dicomwebshould bedicomp10-to-dicomweb(the actual npm package).buntoyarn/npx, aligned screenshot path with actualplaywright.config.ts(chromium-only), and refreshed manual server instructions — all accurate against the config.community.ohif.orgURL to includehttps://, and replaces stale/duplicate directory tree with a clean, accurate overview matching the current repo layout.20.18.1-slimto20.19.0-slim; no logic change.20.18.1-slimto20.19.0-slimto match root Dockerfile.20.18.1-slimto20.19.0-slim.20.18.1-slimto20.19.0-slim.20.18.1-slimto20.19.0-slim.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A([Contributor]) --> B[yarn test:data\ngit submodule update] B --> C{Run mode?} C -->|CI pipeline| D[yarn test:e2e:ci\nnpx playwright test] C -->|Interactive / UI| E[yarn test:e2e:ui\nnpx playwright test --ui] C -->|Headed| F[yarn test:e2e:headed] C -->|Debug| G[yarn test:e2e:debug] C -->|Manual server| H["npx cross-env APP_CONFIG=config/e2e.js\nOHIF_PORT=3335 yarn start\n→ localhost:3335"] H --> I[Playwright reuses\nexisting server] D & E & F & G & I --> J{Tests pass?} J -->|No - screenshots missing| K[Playwright auto-generates\ntests/screenshots/chromium/…] K --> L[Verify ground-truth\nthen re-run] L --> J J -->|Yes| M[npx playwright show-report\ntests/playwright-report]Reviews (1): Last reviewed commit: "Clarify the test docs and refresh the re..." | Re-trigger Greptile