Fix screenshot tests #8289
Fix screenshot tests #8289
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/javascripts/test/puppeteer/dataset_rendering.screenshot.ts (4)
Line range hint
127-131: Consider documenting the rationale for serial test executionThe switch to
test.serialensures deterministic test execution by preventing concurrent tests. While this is good for reliability, it increases total test execution time. Consider adding a comment explaining why serial execution is necessary for these screenshot tests.+// Using serial execution to prevent visual state interference between concurrent tests test.serial("Dataset IDs were retrieved successfully", (t) => {
Line range hint
127-131: Enhance error message for dataset ID validationWhile the validation is good, the error message could be more helpful for debugging.
- t.truthy(datasetNameToId[datasetName], `Dataset ID not found for "${datasetName}"`); + t.truthy( + datasetNameToId[datasetName], + `Failed to retrieve dataset ID for "${datasetName}". Ensure the dataset exists and is accessible.` + );
Line range hint
134-156: Add retry attempt logging for better debuggingThe
withRetrymechanism is good for handling flaky tests, but adding retry attempt logging would help in debugging test failures.test.serial(`it should render dataset ${datasetName} correctly`, async (t) => { + let attemptCount = 0; await withRetry( 3, async () => { + attemptCount++; + console.log(`Attempt ${attemptCount}/3 for dataset ${datasetName}`); const { screenshot, width, height } = await screenshotDataset(
Line range hint
23-26: Enhance unhandled rejection error handlingThe current error handling for unhandled rejections could be improved to preserve stack traces and provide more context.
process.on("unhandledRejection", (err, promise) => { - console.error("Unhandled rejection (promise: ", promise, ", reason: ", err, ")."); + console.error("Unhandled rejection in screenshot tests:"); + if (err instanceof Error) { + console.error(`Error: ${err.message}\nStack: ${err.stack}`); + } else { + console.error("Reason:", err); + } + console.error("Promise:", promise); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
frontend/javascripts/test/screenshots/2017-05-31_mSEM_aniso-test.pngis excluded by!**/*.pngfrontend/javascripts/test/screenshots/2017-05-31_mSEM_scMS109_bk_100um_v01-aniso.pngis excluded by!**/*.pngfrontend/javascripts/test/screenshots/ROI2017_wkw_fallback.pngis excluded by!**/*.pngfrontend/javascripts/test/screenshots/ROI2017_wkw_with_mapping_astrocyte.pngis excluded by!**/*.pngfrontend/javascripts/test/screenshots/annotation_ROI2017_wkw_fallback_null.pngis excluded by!**/*.pngfrontend/javascripts/test/screenshots/annotation_ROI2017_wkw_fallback_segmentation.pngis excluded by!**/*.pngfrontend/javascripts/test/screenshots/connectome_file_test_dataset.pngis excluded by!**/*.pngfrontend/javascripts/test/screenshots/dsA_2.pngis excluded by!**/*.pngfrontend/javascripts/test/screenshots/float_test_dataset.pngis excluded by!**/*.pngfrontend/javascripts/test/screenshots/test-agglomerate-file_with_mapping_link.pngis excluded by!**/*.pngfrontend/javascripts/test/screenshots/test-agglomerate-file_with_meshes_link.pngis excluded by!**/*.png
📒 Files selected for processing (1)
frontend/javascripts/test/puppeteer/dataset_rendering.screenshot.ts(3 hunks)
🔇 Additional comments (1)
frontend/javascripts/test/puppeteer/dataset_rendering.screenshot.ts (1)
30-30: LGTM: URL handling improvements
The URL handling changes ensure consistent URL formatting by:
- Adding a trailing slash to the base URL
- Correctly constructing API endpoint URLs without leading slashes
This prevents double-slash issues in URL resolution.
Also applies to: 117-117
| @@ -114,7 +114,7 @@ test.before("Retrieve dataset ids", async () => { | |||
| 3, | |||
| async () => { | |||
| const options = getDefaultRequestOptions(URL); | |||
| const url = `${URL}/api/datasets/disambiguate/sample_organization/${datasetName}/toId`; | |||
| const url = `${URL}api/datasets/disambiguate/sample_organization/${datasetName}/toId`; | |||
There was a problem hiding this comment.
the rest of the screenshot tests use urljoin to join url parts. that way, double slashes can also be avoided. I'd rather do this then relying on a trailing slash.
There was a problem hiding this comment.
Oh I overlooked that. Thanks for the suggestion. It should be applied now.
I also did a quick test that requesting the URLs still works
…ossos into fix-screenshot-tests
URL of deployed dev instance (used for testing):
Issues: