Optimise core UI test scope#238
Conversation
📝 WalkthroughWalkthroughJest scripts gain ChangesTest Suite Optimization
CI Workflow Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Greptile SummaryThis PR reduces the hot
Confidence Score: 5/5Safe to merge — changes are test-only and CI configuration; no production source files are touched. The PR reorganises test files and CI jobs without modifying any production code. The new waitForMicrotaskAssertions helper is correct for all current synchronous callers. The helper's try/catch would silently skip retries for a future async callback, but a one-line await fix is available and no async callers exist today. packages/core-ui/src/tests/helpers/wait-for-microtasks.js — consider await assertions() to keep the retry logic intact if async callbacks are added in the future. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Before["Before"]
A["data-generator-page.test.js\n(schema helpers + mounted UI + text-mode)"]
B["import-export-workspace.test.js\n(flushAsyncWork fixed-tick)"]
C["shared-schema-definition-view.test.js\n(flushAsyncWork fixed-tick)"]
end
subgraph After["After"]
D["data-generator-page.runtime.test.js\n(no DOM)"]
E["data-generator-page.test.js\n(mounted smoke)"]
F["data-generator-page.text-mode.test.js\n(mounted toggle/lifecycle)"]
G["import-export-workspace.contract.test.js\n(barrel — no DOM)"]
H["import-export-workspace.test.js\n(waitForMicrotaskAssertions)"]
I["shared-schema-definition.contract.test.js\n(barrel — no DOM)"]
J["shared-schema-definition-view.test.js\n(waitForMicrotaskAssertions)"]
K["helpers/wait-for-microtasks.js"]
H --> K
J --> K
end
subgraph CI["CI"]
L["unit-tests"]
M["workspace-tests"]
N["verify-ci gate"]
L --> N
M --> N
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
subgraph Before["Before"]
A["data-generator-page.test.js\n(schema helpers + mounted UI + text-mode)"]
B["import-export-workspace.test.js\n(flushAsyncWork fixed-tick)"]
C["shared-schema-definition-view.test.js\n(flushAsyncWork fixed-tick)"]
end
subgraph After["After"]
D["data-generator-page.runtime.test.js\n(no DOM)"]
E["data-generator-page.test.js\n(mounted smoke)"]
F["data-generator-page.text-mode.test.js\n(mounted toggle/lifecycle)"]
G["import-export-workspace.contract.test.js\n(barrel — no DOM)"]
H["import-export-workspace.test.js\n(waitForMicrotaskAssertions)"]
I["shared-schema-definition.contract.test.js\n(barrel — no DOM)"]
J["shared-schema-definition-view.test.js\n(waitForMicrotaskAssertions)"]
K["helpers/wait-for-microtasks.js"]
H --> K
J --> K
end
subgraph CI["CI"]
L["unit-tests"]
M["workspace-tests"]
N["verify-ci gate"]
L --> N
M --> N
end
Reviews (6): Last reviewed commit: "Harden CI checkout credentials" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
eviltester
left a comment
eviltester
left a comment
There was a problem hiding this comment.
Review: Optimise core UI test scope
I found no bugs, but have a few observations.
|
|
||
| fireEvent.change(fileInput); | ||
| await waitFor(() => { | ||
| await waitForMicrotaskAssertions(() => { |
There was a problem hiding this comment.
Potential timing change: assertions that were previously split between inside and outside waitFor/flushAsyncWork are now all consolidated inside a single waitForMicrotaskAssertions callback. This means every assertion must pass in the same polling iteration. If any chain requires intermediate synchronous side-effects (assertion A must pass before the code that triggers assertion B), this consolidated form could fail where the old code passed. Consider whether any of these converted sites rely on that interleaving.
This also applies to similar consolidations in import-export-workspace.test.js.
| }); | ||
|
|
||
| test('default validation errors surface inline and do not throw', () => { | ||
| const page = createMountedPage({ |
There was a problem hiding this comment.
Inconsistent cleanup: afterEach only calls dom.window.close() but does not clean up global.document and global.window. Compare with shared-schema-definition-view.test.js which explicitly deletes these globals. Since beforeEach unconditionally reassigns them, cross-test contamination is unlikely, but if a test failure prevents beforeEach from running in a subsequent test, stale global references to a closed JSDOM window could cause confusing errors.
| @@ -0,0 +1,467 @@ | |||
| import { jest } from '@jest/globals'; | |||
| import { JSDOM } from 'jsdom'; | |||
| import { faker } from '@faker-js/faker'; | |||
There was a problem hiding this comment.
Default faker/RandExp imports add weight: createMountedPage defaults use real faker and RandExp. Most tests override these with mocks via the overrides parameter, making the real imports unnecessary for the majority of tests. Consider making the defaults use stubs and only pass real faker/RandExp in the subset of tests that exercise domain/faker command resolution.
| throw lastError; | ||
| } | ||
|
|
||
| return assertions(); |
There was a problem hiding this comment.
Dead code: When all maxMicrotasks iterations throw, lastError is set, the if (lastError) branch fires, and this return assertions() is unreachable. It is only reached if maxMicrotasks === 0 (loop never runs). Consider either removing the trailing call or documenting it as a safety net for the zero-iteration edge case.
eviltester
left a comment
eviltester
left a comment
There was a problem hiding this comment.
Review: Optimise core UI test scope
I found no bugs, but have a few observations.
|
|
||
| fireEvent.change(fileInput); | ||
| await waitFor(() => { | ||
| await waitForMicrotaskAssertions(() => { |
There was a problem hiding this comment.
Potential timing change: assertions that were previously split between inside and outside waitFor/flushAsyncWork are now all consolidated inside a single waitForMicrotaskAssertions callback. This means every assertion must pass in the same polling iteration. If any chain requires intermediate synchronous side-effects (assertion A must pass before the code that triggers assertion B), this consolidated form could fail where the old code passed. Consider whether any of these converted sites rely on that interleaving.
This also applies to similar consolidations in import-export-workspace.test.js.
| }); | ||
|
|
||
| test('default validation errors surface inline and do not throw', () => { | ||
| const page = createMountedPage({ |
There was a problem hiding this comment.
Inconsistent cleanup: afterEach only calls dom.window.close() but does not clean up global.document and global.window. Compare with shared-schema-definition-view.test.js which explicitly deletes these globals. Since beforeEach unconditionally reassigns them, cross-test contamination is unlikely, but if a test failure prevents beforeEach from running in a subsequent test, stale global references to a closed JSDOM window could cause confusing errors.
| @@ -0,0 +1,467 @@ | |||
| import { jest } from '@jest/globals'; | |||
| import { JSDOM } from 'jsdom'; | |||
| import { faker } from '@faker-js/faker'; | |||
There was a problem hiding this comment.
Default faker/RandExp imports add weight: createMountedPage defaults use real faker and RandExp. Most tests override these with mocks via the overrides parameter, making the real imports unnecessary for the majority of tests. Consider making the defaults use stubs and only pass real faker/RandExp in the subset of tests that exercise domain/faker command resolution.
| throw lastError; | ||
| } | ||
|
|
||
| return assertions(); |
There was a problem hiding this comment.
Dead code: When all maxMicrotasks iterations throw, lastError is set, the if (lastError) branch fires, and this return assertions() is unreachable. It is only reached if maxMicrotasks === 0 (loop never runs). Consider either removing the trailing call or documenting it as a safety net for the zero-iteration edge case.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/node.js.yml (1)
60-60:⚠️ Potential issue | 🟠 MajorDisable credential persistence in checkout steps.
Both checkout steps on Line 60 and Line 94 are missing
persist-credentials: false. Credentials persist in local git config by default, increasing token-exposure risk during dependency installation steps that may execute arbitrary lifecycle scripts.Suggested fix
- name: Check out repository uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 + with: + persist-credentials: falseApply to both occurrences.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/node.js.yml at line 60, The actions/checkout action is missing the persist-credentials configuration option in both checkout steps, which allows git credentials to remain in the local git config and increases token exposure risk during dependency installation. Add the parameter persist-credentials: false to both occurrences of the actions/checkout action to prevent credentials from being persisted in the local git configuration after the checkout completes.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/node.js.yml:
- Line 60: The actions/checkout action is missing the persist-credentials
configuration option in both checkout steps, which allows git credentials to
remain in the local git config and increases token exposure risk during
dependency installation. Add the parameter persist-credentials: false to both
occurrences of the actions/checkout action to prevent credentials from being
persisted in the local git configuration after the checkout completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 04c60f02-5a2c-4ca7-8e82-26982b2cf4c5
📒 Files selected for processing (4)
.github/workflows/node.js.ymlpackages/core-ui/src/tests/generator/data-generator-page.runtime.test.jspackages/core-ui/src/tests/generator/data-generator-page.text-mode.test.jspackages/core-ui/src/tests/helpers/wait-for-microtasks.js
💤 Files with no reviewable changes (1)
- packages/core-ui/src/tests/helpers/wait-for-microtasks.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core-ui/src/tests/generator/data-generator-page.runtime.test.js
- packages/core-ui/src/tests/generator/data-generator-page.text-mode.test.js
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/node.js.yml (1)
90-124: 📐 Maintainability & Code Quality | 🟠 MajorGate jobs must include
workspace-testsin theirneedslists.The new
workspace-testsjob is cleanly structured and runs in parallel withunit-tests. However, theverify-ciandverify-ci-maingate jobs (lines 283–289 and 298–304) currently list onlychecksandunit-testsin theirneedsarrays. They must also includeworkspace-teststo ensure workspace test failures block merges.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/node.js.yml around lines 90 - 124, The gate jobs verify-ci and verify-ci-main do not include the new workspace-tests job in their needs arrays, which means workspace test failures will not block merges. Locate the verify-ci job (around line 283) and the verify-ci-main job (around line 298), and update their respective needs arrays to include workspace-tests alongside the existing checks and unit-tests dependencies to ensure workspace test failures properly gate pull request merges.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/node.js.yml:
- Around line 90-124: The gate jobs verify-ci and verify-ci-main do not include
the new workspace-tests job in their needs arrays, which means workspace test
failures will not block merges. Locate the verify-ci job (around line 283) and
the verify-ci-main job (around line 298), and update their respective needs
arrays to include workspace-tests alongside the existing checks and unit-tests
dependencies to ensure workspace test failures properly gate pull request
merges.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 230c8629-b416-47f1-b5df-c5db292e8ca7
📒 Files selected for processing (1)
.github/workflows/node.js.yml
|
closes #237 |
Summary
core-uiJest suites by splitting generator tests into mounted smoke, runtime, and text-mode scopes75%Testing
Closes #237
Summary by CodeRabbit
persist-credentials: falsefor multiple steps.--maxWorkers=75%and removed the benchmark:tabulator script.