Update screenshot tests to use Vitest #8553
Conversation
📝 Walkthrough""" WalkthroughThis set of changes migrates the frontend JavaScript/TypeScript test suite from the AVA testing framework to Vitest, specifically targeting Puppeteer-based screenshot and E2E tests. The migration includes refactoring test files to use Vitest's lifecycle hooks, assertion styles, and context handling. Supporting utilities and helper functions are updated to remove AVA-specific constructs and adopt Vitest-compatible patterns. The Changes
Assessment against linked issues
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)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
| } | ||
| `; | ||
|
|
||
| exports[`Project API > createProject and deleteProject 1`] = ` |
There was a problem hiding this comment.
The test suite was renamed from Project API to Project API (E2E). Hence, the snapshots were marked and reported as obsolete by vitest.
| t.context.browser = browser; | ||
| console.log(`\nBrowserStack Session Id ${await getBrowserstackSessionId(browser)}\n`); | ||
| } | ||
| export async function setupBeforeEach(context: ScreenshotTestContext, semaphore?: Semaphore) { |
There was a problem hiding this comment.
This change is mostly identical just with different indentation. I split the method setupBeforeEachAndAfterEach() into two separate setupBeforeEach() and setupAfterEach() methods.
hotzenklotz
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
frontend/javascripts/test/puppeteer/dtype_datasets_rendering.screenshot.ts (1)
268-277: Minor: reusechangedPixelsvariable instead of shadowingInside the selective-segment branch you redeclare
const changedPixels, shadowing the outer variable.
This is harmless but a little confusing; consider re-using the original name or renaming the inner variable.-const changedPixels = await compareScreenshot( +const selectiveChangedPixels = await compareScreenshot(frontend/javascripts/test/puppeteer/dataset_rendering.screenshot.ts (2)
188-200: Mismatch between file name used for screenshots and error messageThe diff/new screenshots are saved under
annotation_${datasetName}_${fallbackLabel}.*.png
but the failure message refers to
annotation_${datasetName}_${fallbackLayerName}.*.png.This confuses debugging because the suggested file names do not exist.
-`Annotation for dataset with name: "${datasetName}" does not look the same, see annotation_${datasetName}_${fallbackLayerName}.diff.png for the difference and annotation_${datasetName}_${fallbackLayerName}.new.png for the new screenshot.` +`Annotation for dataset with name: "${datasetName}" does not look the same, see annotation_${datasetName}_${fallbackLabel}.diff.png for the difference and annotation_${datasetName}_${fallbackLabel}.new.png for the new screenshot.`
118-157:withRetrysilently skips exceptions – consider catching errors to actually retry
withRetryonly retries when the callback resolves tofalse. Any thrown exception aborts the whole test immediately, defeating the purpose of the retry wrapper (common when Puppeteer navigation fails or BrowserStack hiccups).At each call site you could wrap the logic:
await withRetry(3, async () => { try { // existing screenshot logic… return isPixelEquivalent(changedPixels, width, height); } catch { return false; // force retry } }, (condition) => { … });Alternatively, enhance
withRetryin the helper to do the try/catch once centrally.
This will save flaky CI runs without masking genuine pixel mismatches.Also applies to: 162-202, 206-243, 245-318, 320-357
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/javascripts/test/backend-snapshot-tests/__snapshots__/projects.e2e.ts.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
frontend/javascripts/test/backend-snapshot-tests/projects.e2e.ts(1 hunks)frontend/javascripts/test/e2e-setup.ts(1 hunks)frontend/javascripts/test/puppeteer/dataset_rendering.screenshot.ts(2 hunks)frontend/javascripts/test/puppeteer/dataset_rendering.wkorg_screenshot.ts(2 hunks)frontend/javascripts/test/puppeteer/dataset_rendering_helpers.ts(15 hunks)frontend/javascripts/test/puppeteer/dtype_datasets_rendering.screenshot.ts(3 hunks)package.json(2 hunks)tools/test.sh(0 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- tools/test.sh
🧰 Additional context used
🧬 Code Graph Analysis (3)
frontend/javascripts/test/puppeteer/dataset_rendering.wkorg_screenshot.ts (2)
frontend/javascripts/test/puppeteer/dataset_rendering_helpers.ts (7)
process(22-22)ScreenshotTestContext(440-443)setupBeforeEach(445-489)setupAfterEach(491-496)withRetry(421-437)getNewPage(406-419)screenshotDatasetView(153-163)frontend/javascripts/test/puppeteer/screenshot_helpers.ts (2)
compareScreenshot(87-143)isPixelEquivalent(30-35)
frontend/javascripts/test/puppeteer/dtype_datasets_rendering.screenshot.ts (4)
frontend/javascripts/test/puppeteer/dataset_rendering_helpers.ts (9)
ScreenshotTestContext(440-443)setupBeforeEach(445-489)setupAfterEach(491-496)writeDatasetNameToIdMapping(51-71)createAnnotationForDatasetScreenshot(73-76)getNewPage(406-419)withRetry(421-437)screenshotDataset(85-99)screenshotTracingView(352-404)frontend/javascripts/oxalis/model/actions/settings_actions.ts (3)
updateDatasetSettingAction(65-73)updateTemporarySettingAction(75-83)updateLayerSettingAction(91-101)frontend/javascripts/oxalis/model/actions/flycam_actions.ts (2)
setPositionAction(76-81)setZoomStepAction(70-74)frontend/javascripts/test/puppeteer/screenshot_helpers.ts (3)
compareScreenshot(87-143)SCREENSHOTS_BASE_PATH(6-9)isPixelEquivalent(30-35)
frontend/javascripts/test/puppeteer/dataset_rendering.screenshot.ts (2)
frontend/javascripts/test/puppeteer/dataset_rendering_helpers.ts (11)
ScreenshotTestContext(440-443)setupBeforeEach(445-489)setupAfterEach(491-496)writeDatasetNameToIdMapping(51-71)withRetry(421-437)getNewPage(406-419)screenshotDataset(85-99)screenshotAnnotation(101-126)screenshotDatasetWithMapping(165-188)screenshotDatasetWithMappingLink(189-209)screenshotSandboxWithMappingLink(210-220)frontend/javascripts/test/puppeteer/screenshot_helpers.ts (3)
compareScreenshot(87-143)SCREENSHOTS_BASE_PATH(6-9)isPixelEquivalent(30-35)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (11)
frontend/javascripts/test/backend-snapshot-tests/projects.e2e.ts (1)
22-22: Style consistency additionThis blank line addition improves readability by maintaining consistent spacing between the
beforeEachhook and the test cases, aligning with the Vitest migration's style conventions.frontend/javascripts/test/e2e-setup.ts (1)
11-11: Improved mocking behavior documentationThe updated comment provides clearer explanation of the mocking behavior for the request library, correctly distinguishing between unit tests (globally mocked) and E2E tests (using original implementation). This clarification is valuable during the testing framework migration.
vitest.config.ts (2)
8-12: Test configuration properly updated for Puppeteer screenshot testsThe configuration now correctly includes Puppeteer screenshot test files by extending the include patterns and removing the previous exclusions. This change is essential for the AVA to Vitest migration of the screenshot tests.
#!/bin/bash # Verify if all screenshot test files are covered by the new include patterns # Find all wkorg_screenshot and screenshot test files in the puppeteer directory echo "Screenshot test files that should be included:" find ./frontend/javascripts/test/puppeteer -name "*.wkorg_screenshot.ts" -o -name "*.screenshot.ts" echo -e "\nVerifying if patterns match all test files:" find ./frontend/javascripts/test/puppeteer -name "*.wkorg_screenshot.ts" -o -name "*.screenshot.ts" | xargs -I{} basename {} | grep -E "\.((wkorg_screenshot)|(screenshot))\.ts$"
16-16: Format consistency in coverage reporter configurationSwitching from single to double quotes in the coverage reporter array maintains consistent formatting style with the rest of the configuration file.
frontend/javascripts/test/puppeteer/dataset_rendering.wkorg_screenshot.ts (4)
6-9: Updated imports to support Vitest test contextThese import changes correctly adapt to the new helper function structure created for Vitest, importing the type
ScreenshotTestContextand the separatedsetupBeforeEachandsetupAfterEachfunctions.
12-12: Added Vitest test constructsAppropriately replaced AVA imports with Vitest's testing constructs, supporting the migration from AVA to Vitest testing framework.
45-53: Test structure migrated to Vitest formatSuccessfully refactored the test structure to use Vitest's
describeblock with properbeforeEachandafterEachhooks. The implementation correctly uses the new helper functionssetupBeforeEachandsetupAfterEachwith the appropriate test context typing.
54-91: Test case properly migrated to Vitest formatThe test case has been successfully migrated from AVA's
test.serialto Vitest'sit.sequentialwith proper context handling. The assertion style has been updated from AVA'st.trueto Vitest'sexpect().toBe(true)while maintaining the same testing logic and error messages.package.json (1)
92-93: Screenshot tests may run concurrently across workers – consider disabling parallelism
vitest runwill spawn several worker-threads by default.
Because every screenshot test launches a full Chrome/BrowserStack session, parallel workers can
- exceed the BrowserStack concurrency limit,
- overload the local machine (many Chrome instances),
- ignore the semaphore in your test file (it is only process-local and therefore not shared across workers).
Align the screenshot script with the existing
test-e2escript by turning off parallelism:-"test-screenshot": "vitest run --testTimeout=300000 frontend/javascripts/test/puppeteer/**/*.screenshot.ts", -"test-wkorg-screenshot": "vitest run --testTimeout=300000 frontend/javascripts/test/puppeteer/**/*.wkorg_screenshot.ts", +"test-screenshot": "vitest run --fileParallelism=false --threads=false --testTimeout=300000 frontend/javascripts/test/puppeteer/**/*.screenshot.ts", +"test-wkorg-screenshot": "vitest run --fileParallelism=false --threads=false --testTimeout=300000 frontend/javascripts/test/puppeteer/**/*.wkorg_screenshot.ts",Please verify that turning parallelism off is acceptable for CI runtime.
frontend/javascripts/test/puppeteer/dtype_datasets_rendering.screenshot.ts (1)
189-197: Semaphore does not prevent cross-worker concurrency
beforeEachacquires aSemaphore(1), but the semaphore instance lives only inside the current worker process.
If Vitest runs this file in multiple workers (default), other workers will create their own semaphore with the same capacity, so several browsers can start in parallel.Unless you disable worker threads (see package.json comment) you can still get race conditions / BrowserStack quota errors.
No code change needed here if you adopt
--threads=false, otherwise use a cross-process lock (e.g.proper-lockfileon the FS).frontend/javascripts/test/puppeteer/dataset_rendering_helpers.ts (1)
445-489: Semaphore release may be skipped on test failure
setupAfterEachreleases the semaphore, but it only runs after a test hook
completes successfully. If the test crashes before finishing (e.g., unhandled
exception inside Puppeteer),afterEachis still executed, but if the process
crashes or Vitest is interrupted the lock file may persist across retries.Consider wrapping the test body in
try/finallyor moving the release logic
into afinallyblock inside the test body to guarantee cleanup.
| import { vi, type TestContext } from "vitest"; | ||
|
|
||
| vi.mock("libs/request", async (importOriginal) => { | ||
| // The request lib is globally mocked for the unit tests. In the screenshot tests, we actually want to run the proper fetch calls so we revert to the original implementation | ||
| return await importOriginal(); | ||
| }); |
There was a problem hiding this comment.
Check correctness of vi.mock factory signature
Vitest passes no argument to the factory function – to access the real module you should call
vi.importActual() inside the factory.
Calling the (undefined) importOriginal parameter will throw at runtime.
-vi.mock("libs/request", async (importOriginal) => {
- return await importOriginal();
+vi.mock("libs/request", async () => {
+ const actual = await vi.importActual<typeof import("libs/request")>("libs/request");
+ return actual;
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { vi, type TestContext } from "vitest"; | |
| vi.mock("libs/request", async (importOriginal) => { | |
| // The request lib is globally mocked for the unit tests. In the screenshot tests, we actually want to run the proper fetch calls so we revert to the original implementation | |
| return await importOriginal(); | |
| }); | |
| import { vi, type TestContext } from "vitest"; | |
| vi.mock("libs/request", async () => { | |
| // The request lib is globally mocked for the unit tests. In the screenshot tests, we actually want to run the proper fetch calls so we revert to the original implementation | |
| const actual = await vi.importActual<typeof import("libs/request")>("libs/request"); | |
| return actual; | |
| }); |
| export async function setupBeforeEach(context: ScreenshotTestContext, semaphore?: Semaphore) { | ||
| if (semaphore) { | ||
| context.release = await semaphore.acquire(); | ||
| } | ||
| if (USE_LOCAL_CHROME) { | ||
| // Use this for connecting to local Chrome browser instance | ||
| context.browser = await puppeteer.launch({ | ||
| args: HEADLESS | ||
| ? [ | ||
| "--headless=false", | ||
| "--hide-scrollbars", | ||
| "--no-sandbox", | ||
| "--disable-setuid-sandbox", | ||
| "--disable-dev-shm-usage", | ||
| "--use-angle=gl-egl", | ||
| ] | ||
| : [], | ||
| headless: HEADLESS ? "new" : false, // use "new" to suppress warnings | ||
| dumpio: true, | ||
| executablePath: "/usr/bin/google-chrome", // Linux; this might need to be adapted to your local setup | ||
| // executablePath: "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome", // Mac | ||
| }); | ||
| } else { | ||
| checkBrowserstackCredentials(); | ||
|
|
||
| const caps = { | ||
| browser: "chrome", | ||
| browser_version: "latest", | ||
| os: "os x", | ||
| os_version: "mojave", | ||
| name: context.task.name, // add test name to BrowserStack session | ||
| "browserstack.username": process.env.BROWSERSTACK_USERNAME, | ||
| "browserstack.accessKey": process.env.BROWSERSTACK_ACCESS_KEY, | ||
| }; | ||
| const browser = await puppeteer.connect({ | ||
| browserWSEndpoint: `ws://cdp.browserstack.com/puppeteer?caps=${encodeURIComponent( | ||
| JSON.stringify(caps), | ||
| )}`, | ||
| }); | ||
| context.browser = browser; | ||
| console.log(`\nBrowserStack Session Id ${await getBrowserstackSessionId(browser)}\n`); | ||
| } | ||
|
|
||
| console.log(`\nRunning chrome version ${await t.context.browser.version()}\n`); | ||
| global.Headers = Headers; | ||
| global.fetch = fetch; | ||
| global.Request = Request; | ||
| global.Response = Response; | ||
| // @ts-expect-error ts-migrate(7017) FIXME: Element implicitly has an 'any' type because type ... Remove this comment to see the full error message | ||
| global.FetchError = FetchError; | ||
| }); | ||
| console.log(`\nRunning chrome version ${await context.browser.version()}\n`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
HEADLESS flags contradict each other
When HEADLESS === true you pass the argument --headless=false and
headless: "new". The CLI flag overrides the option object, resulting in a
non-headless browser although the constant says otherwise.
Either invert the flag or drop the argument list:
-args: HEADLESS
- ? [
- "--headless=false",
+args: HEADLESS
+ ? [
+ "--headless=new",(or build the args array only when !HEADLESS).
This avoids confusion and makes local debugging easier.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function setupBeforeEach(context: ScreenshotTestContext, semaphore?: Semaphore) { | |
| if (semaphore) { | |
| context.release = await semaphore.acquire(); | |
| } | |
| if (USE_LOCAL_CHROME) { | |
| // Use this for connecting to local Chrome browser instance | |
| context.browser = await puppeteer.launch({ | |
| args: HEADLESS | |
| ? [ | |
| "--headless=false", | |
| "--hide-scrollbars", | |
| "--no-sandbox", | |
| "--disable-setuid-sandbox", | |
| "--disable-dev-shm-usage", | |
| "--use-angle=gl-egl", | |
| ] | |
| : [], | |
| headless: HEADLESS ? "new" : false, // use "new" to suppress warnings | |
| dumpio: true, | |
| executablePath: "/usr/bin/google-chrome", // Linux; this might need to be adapted to your local setup | |
| // executablePath: "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome", // Mac | |
| }); | |
| } else { | |
| checkBrowserstackCredentials(); | |
| const caps = { | |
| browser: "chrome", | |
| browser_version: "latest", | |
| os: "os x", | |
| os_version: "mojave", | |
| name: context.task.name, // add test name to BrowserStack session | |
| "browserstack.username": process.env.BROWSERSTACK_USERNAME, | |
| "browserstack.accessKey": process.env.BROWSERSTACK_ACCESS_KEY, | |
| }; | |
| const browser = await puppeteer.connect({ | |
| browserWSEndpoint: `ws://cdp.browserstack.com/puppeteer?caps=${encodeURIComponent( | |
| JSON.stringify(caps), | |
| )}`, | |
| }); | |
| context.browser = browser; | |
| console.log(`\nBrowserStack Session Id ${await getBrowserstackSessionId(browser)}\n`); | |
| } | |
| console.log(`\nRunning chrome version ${await t.context.browser.version()}\n`); | |
| global.Headers = Headers; | |
| global.fetch = fetch; | |
| global.Request = Request; | |
| global.Response = Response; | |
| // @ts-expect-error ts-migrate(7017) FIXME: Element implicitly has an 'any' type because type ... Remove this comment to see the full error message | |
| global.FetchError = FetchError; | |
| }); | |
| console.log(`\nRunning chrome version ${await context.browser.version()}\n`); | |
| } | |
| export async function setupBeforeEach(context: ScreenshotTestContext, semaphore?: Semaphore) { | |
| if (semaphore) { | |
| context.release = await semaphore.acquire(); | |
| } | |
| if (USE_LOCAL_CHROME) { | |
| // Use this for connecting to local Chrome browser instance | |
| context.browser = await puppeteer.launch({ | |
| args: HEADLESS | |
| ? [ | |
| "--headless=new", | |
| "--hide-scrollbars", | |
| "--no-sandbox", | |
| "--disable-setuid-sandbox", | |
| "--disable-dev-shm-usage", | |
| "--use-angle=gl-egl", | |
| ] | |
| : [], | |
| headless: HEADLESS ? "new" : false, // use "new" to suppress warnings | |
| dumpio: true, | |
| executablePath: "/usr/bin/google-chrome", // Linux; this might need to be adapted to your local setup | |
| // executablePath: "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome", // Mac | |
| }); | |
| } else { | |
| checkBrowserstackCredentials(); | |
| const caps = { | |
| browser: "chrome", | |
| browser_version: "latest", | |
| os: "os x", | |
| os_version: "mojave", | |
| name: context.task.name, // add test name to BrowserStack session | |
| "browserstack.username": process.env.BROWSERSTACK_USERNAME, | |
| "browserstack.accessKey": process.env.BROWSERSTACK_ACCESS_KEY, | |
| }; | |
| const browser = await puppeteer.connect({ | |
| browserWSEndpoint: `ws://cdp.browserstack.com/puppeteer?caps=${encodeURIComponent( | |
| JSON.stringify(caps), | |
| )}`, | |
| }); | |
| context.browser = browser; | |
| console.log(`\nBrowserStack Session Id ${await getBrowserstackSessionId(browser)}\n`); | |
| } | |
| console.log(`\nRunning chrome version ${await context.browser.version()}\n`); | |
| } |
There was a problem hiding this comment.
@philippotto I did not change the headless browser setup but the CodeRabbit comment is valid. Perhaps we remove line 454 ("--headless=false")?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I remember that there was some api change in puppeteer that required this weird headless: "new" stuff. back then, I briefly fiddled with it and the current way worked for both headless and not headless (useful for seeing the actual browser window). I cannot judge whether coderabbits suggestion works, but feel free to try it out. just set the local chrome flag to true and test both headless and "headfull".
| describe("Dataset Rendering", () => { | ||
| beforeEach<ScreenshotTestContext>(async (context) => { | ||
| await setupBeforeEach(context); | ||
| }); | ||
|
|
||
| afterEach<ScreenshotTestContext>(async (context) => { | ||
| await setupAfterEach(context); | ||
| }); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider limiting browser concurrency to avoid resource exhaustion
beforeEach launches a fresh Chromium instance for every single test case. Even though each individual test is flagged as it.sequential, Vitest may still run multiple files in parallel, which can easily exhaust local CI resources or BrowserStack quotas.
If you intend to guarantee a single browser at any given time, pass in the semaphore from dataset_rendering_helpers (or create a simple one here) and acquire it in setupBeforeEach.
Example patch (sketch):
-beforeEach<ScreenshotTestContext>(async (context) => {
- await setupBeforeEach(context);
-});
+const browserSemaphore = new Semaphore(1); // 1 == fully serialise across files
+
+beforeEach<ScreenshotTestContext>(async (context) => {
+ await setupBeforeEach(context, browserSemaphore);
+});This keeps the behaviour identical on a developer machine while preventing accidental parallel launches in CI.
🏁 Script executed:
#!/bin/bash
# Locate the helpers file and inspect setupBeforeEach definition and signature
rg -n "dataset_rendering_helpers"
rg -n "setupBeforeEach" -n Length of output: 1191
🏁 Script executed:
#!/bin/bash
# Inspect Semaphore import and setupBeforeEach/AfterEach definitions
rg -n -A5 -B5 "import.*Semaphore" frontend/javascripts/test/puppeteer/dataset_rendering_helpers.ts
rg -n -A5 -B5 "export async function setupBeforeEach" frontend/javascripts/test/puppeteer/dataset_rendering_helpers.ts
rg -n -A5 -B5 "export async function setupAfterEach" frontend/javascripts/test/puppeteer/dataset_rendering_helpers.tsLength of output: 1749
Limit browser concurrency to prevent CI resource exhaustion
To serialize Chromium launches across this test file (and avoid exhausting CI or BrowserStack quotas), import and instantiate a Semaphore and pass it into your existing setupBeforeEach call—mirroring the pattern used in dtype_datasets_rendering.screenshot.ts.
• File: frontend/javascripts/test/puppeteer/dataset_rendering.screenshot.ts
– Add an import for the runtime Semaphore class
– Instantiate a single-permit semaphore at module scope
– Pass that semaphore into setupBeforeEach
--- a/frontend/javascripts/test/puppeteer/dataset_rendering.screenshot.ts
+++ b/frontend/javascripts/test/puppeteer/dataset_rendering.screenshot.ts
@@ 1,6 +1,9 @@
import { describe, it, beforeEach, afterEach } from "vitest";
import { setupBeforeEach, setupAfterEach } from "./dataset_rendering_helpers";
+import Semaphore from "semaphore-promise";
+
+const browserSemaphore = new Semaphore(1);
describe("Dataset Rendering", () => {
- beforeEach<ScreenshotTestContext>(async (context) => {
- await setupBeforeEach(context);
- });
+ beforeEach<ScreenshotTestContext>(async (context) => {
+ await setupBeforeEach(context, browserSemaphore);
+ });
afterEach<ScreenshotTestContext>(async (context) => {
await setupAfterEach(context);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe("Dataset Rendering", () => { | |
| beforeEach<ScreenshotTestContext>(async (context) => { | |
| await setupBeforeEach(context); | |
| }); | |
| afterEach<ScreenshotTestContext>(async (context) => { | |
| await setupAfterEach(context); | |
| }); | |
| // frontend/javascripts/test/puppeteer/dataset_rendering.screenshot.ts | |
| import { describe, it, beforeEach, afterEach } from "vitest"; | |
| import { setupBeforeEach, setupAfterEach } from "./dataset_rendering_helpers"; | |
| +import Semaphore from "semaphore-promise"; | |
| + | |
| +const browserSemaphore = new Semaphore(1); | |
| describe("Dataset Rendering", () => { | |
| - beforeEach<ScreenshotTestContext>(async (context) => { | |
| - await setupBeforeEach(context); | |
| - }); | |
| + beforeEach<ScreenshotTestContext>(async (context) => { | |
| + await setupBeforeEach(context, browserSemaphore); | |
| + }); | |
| afterEach<ScreenshotTestContext>(async (context) => { | |
| await setupAfterEach(context); | |
| }); | |
| // …rest of your tests… | |
| }); |
There was a problem hiding this comment.
@philippotto What do you think about this? This sounds reasonable to me.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I initially added the semaphore logic to the dtype test, because I tested it locally and in parallel, but didn't have enough ram for running all tests in parallel (in the end, I apparently committed 1 for the semaphore value which is identical to running them serially).
in theory, running all these specs in parallel should work fine with browserstack (so, no sequential and no semaphore). this would probably make the tests a bit faster.
to summarize I see these two options:
A) make/keep everything sequential and remove the semaphore
B) make/keep everything concurrent but use the semaphore to limit it to sane limits (e.g., 2 if localhost else 5?)
not sure whether B will also work for the normal dataset_rendering.screenshot.ts (because it also changes the view config, I think), but it should definitely work for the dtypes specs.
There was a problem hiding this comment.
Ok. Then the default vitest behavior (sequential execution of all tests within on suite) should almost cover case A) . I can remove the semaphore to make the code easier.
There was a problem hiding this comment.
I removed the semaphore and switched to vitest's test.for API instead of loops to ensure proper test setup/teardown. https://vitest.dev/api/#test-for
…ot.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Updated screenshot tests to use
vitestinstead ofava. Removedtools/test.shand all references to it. Running the screenshot tests on BrowserStack took 5min 25s which is still faster then our nightly run (ca. 9min).Removed the following dev dependencies:
avanode-fetchc8(frontend testing coverage; vitest uses the node.js builtin v8 coverage)mock-requirevitesthas an experimental feature called Browser Mode, which will run a test suite inside a browser either throughplaywrightorwebdriverio. Since we are usingpuppeteerI decided to not use browser mode and not change our current setup. Perhaps, this is feature is something we might want to explore in the future.Steps to test:
WK_AUTH_TOKEN=... BROWSERSTACK_USERNAME=... BROWSERSTACK_ACCESS_KEY=... yarn test-screenshotWK_AUTH_TOKEN=... BROWSERSTACK_USERNAME=... BROWSERSTACK_ACCESS_KEY=... yarn test-wkorg-screenshotTODOs:
Find a good way to update screenshots and snapshots. Maybe a dedicated GitHub Actions workflow.(Follow up)Issues:
(Please delete unneeded items, merge only when none are left open)