refactor(vitest): convert to multi-project workspace with dynamic path resolution#808
refactor(vitest): convert to multi-project workspace with dynamic path resolution#808senutpal wants to merge 5 commits intoCircuitVerse:mainfrom
Conversation
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds a Vitest workspace and CI test workflow, updates test wiring and many simulator tests, and adjusts build/config files. Key facts: new vitest.workspace.ts exporting two per-version workspace configs (src, v1); package.json Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@vitest.workspace.ts`:
- Around line 8-72: The workspace configs in vitest.workspace.ts extend the base
vite.config.ts which reads process.env.VITE_SIM_VERSION (defaulting to "v0"), so
the vueI18n plugin in the base config will always load v0 locales for every
workspace; fix by ensuring each workspace sets VITE_SIM_VERSION appropriately or
by moving version-dependent plugin configuration into each workspace block:
update the v1 workspace to set VITE_SIM_VERSION="v1" (or equivalent
per-workspace env support) or remove the vueI18n/plugin config from
vite.config.ts and add it into each workspace override so the plugin uses the
workspace's resolve.alias and correct version-specific locales (refer to
process.env.VITE_SIM_VERSION, vite.config.ts, and the workspace objects in
vitest.workspace.ts).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.jsonvite.config.tsvitest.workspace.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-07T19:39:24.443Z
Learnt from: naman79820
Repo: CircuitVerse/cv-frontend-vue PR: 729
File: package.json:56-56
Timestamp: 2026-01-07T19:39:24.443Z
Learning: In Node 18 environments (e.g., this repo cv-frontend-vue), pin cross-env to 7.x (e.g., ^7.0.3) because cross-env 10.x requires Node >= 20. When editing package.json, ensure cross-env version stays within 7.x to maintain compatibility with Node 18.
Applied to files:
package.json
🔇 Additional comments (4)
vite.config.ts (2)
35-45: LGTM: formatting-only change.
No behavioral impact expected; reads cleaner.
63-74: Good consolidation of test config into workspace.
Keeping Vitest config in one place should prevent conflicts.package.json (1)
14-14: LGTM: test script aligned with workspace config.vitest.workspace.ts (1)
1-27: Nice workspace layout and cross‑platform path resolution.
Per‑project alias + setupFiles look clean and isolated.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
senutpal
force-pushed
the
refactor/test-workspace-configuration
branch
from
7a3a363 to
93ad29e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/simulator/src/node.js (1)
399-520: Address Biome noFallthrough and noRedeclare lint errors in switch statement.The function has two
var ideclarations at lines 393 and 441 within the same scope, which triggers the noRedeclare rule. The switch statement at lines 441-520 contains intentional fallthroughs withoutbiome-ignoredirectives, which triggers noFallthrough warnings. Change the loop counter at line 441 fromvar itolet ito avoid the redeclaration issue, and addbiome-ignore lint/suspicious/noFallthroughSwitchClausecomments before the fallthrough cases at lines 472 (NODE_INPUT) and 482 (NODE_INTERMEDIATE).v1/src/simulator/src/node.js (1)
399-520: Resolve switch fallthroughs and variable redeclarations flagged by Biome.The switch statement has intentional fallthroughs between
NODE_OUTPUTandNODE_INPUT, and betweenNODE_INPUTandNODE_INTERMEDIATE. Additionally, the function declaresvar itwice in the same scope (lines 393 and 441), triggering Biome'snoRedeclarerule. Both issues will cause lint failures.Add
// biome-ignore lint/suspicious/noFallthroughSwitchClausebefore each fallthrough case and change bothvar ideclarations tolet ito resolve this.Suggested fix
- for (var i = 0; i < this.connections.length; i++) { + for (let i = 0; i < this.connections.length; i++) { if (this.connections[i].value !== undefined) { this.connections[i].value = undefined simulationArea.simulationQueue.add(this.connections[i]) } } ... - for (var i = 0; i < this.connections.length; i++) { + for (let i = 0; i < this.connections.length; i++) { const node = this.connections[i] switch (node.type) { + // biome-ignore lint/suspicious/noFallthroughSwitchClause case NODE_OUTPUT: ... // Fallthrough. NODE_OUTPUT propagates like a contention checked NODE_INPUT + // biome-ignore lint/suspicious/noFallthroughSwitchClause case NODE_INPUT: ... // Fallthrough. NODE_INPUT propagates like a bitwidth checked NODE_INTERMEDIATE + // biome-ignore lint/suspicious/noFallthroughSwitchClause case NODE_INTERMEDIATE: ... default: break } }src/simulator/spec/gates.spec.js (1)
55-104: Avoid test-order coupling by loading circuit data in setup.
runAlldefaults toglobalScope, which is initialized byload(circuitData). If the “load circuitData” test is skipped or order changes, later tests can fail or flake. Load once inbeforeAll(or per test) to make execution order irrelevant.🛠️ Suggested fix
beforeAll(async () => { pinia = createPinia() setActivePinia(pinia) @@ mount(simulator, { global: { plugins: [pinia, router, i18n, vuetify], }, attachTo: elem, }) setup() + load(circuitData) })src/simulator/spec/decoders-plexers.spec.js (1)
55-104: Remove the implicit dependency on test order.
runAllrelies onglobalScope, which is initialized byload(circuitData). If the load test doesn’t run first (or is filtered), later tests can fail. Load the circuit data inbeforeAll(or in each test) to make tests independent.🛠️ Suggested fix
beforeAll(async () => { pinia = createPinia() setActivePinia(pinia) @@ mount(simulator, { global: { plugins: [pinia, router, i18n, vuetify], }, attachTo: elem, }) setup() + load(circuitData) })src/simulator/spec/sequential.spec.js (1)
55-104: Make tests independent by loading the circuit data in setup.
Later tests depend onglobalScopebeing initialized byload(circuitData). If the “load circuitData” test is skipped or reordered, the rest can fail. Load once inbeforeAll(or per test) to avoid order coupling.🛠️ Suggested fix
beforeAll(async () => { pinia = createPinia() setActivePinia(pinia) @@ mount(simulator, { global: { plugins: [pinia, router, i18n, vuetify], }, attachTo: elem, }) setup() + load(circuitData) })
🤖 Fix all issues with AI agents
In `@src/simulator/spec/vitestSetup.ts`:
- Around line 31-100: There are two afterAll hooks causing a timing gap where
cleanup happens before isTearingDown is set, so expected teardown errors are not
suppressed; merge the teardown logic into a single afterAll that first sets
isTearingDown = true, then performs cleanup (clear timers, useRealTimers,
clearAllMocks, restoreAllMocks) and only after that unregisters the process
handlers (process.off for 'unhandledRejection' and 'uncaughtException'); update
or remove the earlier afterAll block so all cleanup uses the consolidated
afterAll referencing isTearingDown, handleUnhandledRejection,
handleUncaughtException, and suppressedPatterns.
In `@v1/src/simulator/spec/bitConvertor.spec.js`:
- Line 52: The describe block currently reads "data dir working" but these tests
are for bitConvertor; rename the describe block string from "data dir working"
to "bitConvertor" so the test suite accurately reflects the tested module
(update the describe declaration that currently appears as describe('data dir
working', ...) to describe('bitConvertor', ...)).
In `@v1/src/simulator/spec/combinationalAnalysis.spec.js`:
- Around line 114-120: The test mutates the shared testData.AndGate object
causing cross-test contamination; fix it by creating a local deep clone of
testData.AndGate (e.g., using structuredClone or
JSON.parse(JSON.stringify(testData.AndGate))) at the start of the test, modify
labels on that cloned object instead of testData.AndGate, and pass the clone to
runAll so the original testData stays unchanged (this prevents leaking state
into gates.spec.js which imports the same module).
In `@v1/src/simulator/spec/vitestSetup.ts`:
- Around line 92-95: The teardown sets isTearingDown after unregistering the
error handlers, making shouldSuppressError ineffective during cleanup; in the
afterAll block set isTearingDown = true before performing any cleanup that may
trigger handlers (so shouldSuppressError will see the flag), keep the handlers
(handleUnhandledRejection and handleUncaughtException) registered while cleanup
runs, and only call process.off('unhandledRejection', handleUnhandledRejection)
and process.off('uncaughtException', handleUncaughtException) after cleanup
completes so the handlers can suppress errors during teardown.
🧹 Nitpick comments (9)
src/simulator/spec/bitConvertor.spec.js (1)
112-116: Prefer deterministic input in tests.Random inputs make failures harder to reproduce.
🧪 Suggested tweak
- const randomBaseValue = Math.floor(Math.random() * 100) - console.log('Testing for Base Value --> ', randomBaseValue) - expect(() => setBaseValues(randomBaseValue)).not.toThrow() + const baseValue = 42 + expect(() => setBaseValues(baseValue)).not.toThrow()src/simulator/spec/combinationalAnalysis.spec.js (2)
92-92: Consider usingwindow.globalScopefor consistency with TypeScript declarations.Based on learnings,
globalScopeshould be declared on the window object rather than usingglobal.globalScope. This is inconsistent with the pattern established invitestSetup.tswhere window properties are properly typed and assigned.Suggested change
- global.globalScope = global.globalScope || {} + window.globalScope = window.globalScope || {}
94-101: Mounted component reference is discarded; consider cleanup.The
mount()call returns a wrapper that isn't stored. While this may work forbeforeAll, if tests modify component state, there's no way to access or unmount it. Consider storing the wrapper if you need to interact with it or ensure proper cleanup.v1/src/simulator/spec/vitestSetup.ts (1)
37-42: Duplicate cleanup calls across twoafterAllhooks.The cleanup methods (
vi.clearAllTimers(),vi.useRealTimers(),vi.clearAllMocks(),vi.restoreAllMocks()) are called in bothafterAllblocks (lines 38-41 and 96-99). This redundancy is harmless but unnecessary. Consider consolidating into a singleafterAllhook.Consolidate afterAll hooks
-// Restore real timers and cleanup after all tests -afterAll(() => { - vi.clearAllTimers() - vi.useRealTimers() - vi.clearAllMocks() - vi.restoreAllMocks() -}) - // Patterns for errors that should be suppressed during teardown const suppressedPatterns = [ ... ] ... // Unregister error handlers and suppress expected teardown errors afterAll(() => { isTearingDown = true process.off('unhandledRejection', handleUnhandledRejection) process.off('uncaughtException', handleUncaughtException) vi.clearAllTimers() vi.useRealTimers() vi.clearAllMocks() vi.restoreAllMocks() })Also applies to: 92-100
v1/src/simulator/spec/decoders-plexers.spec.js (1)
15-100: Consider extracting shared test scaffolding to reduce duplication.This entire block (mocks,
beforeAllsetup with Pinia/Router/mount) is duplicated across multiple spec files (gates.spec.js,subCircuit.spec.js,bitConvertor.spec.js, etc.). Since the workspace already has avitestSetup.ts, consider moving common mocks and setup utilities there or to a shared test helper module.This would:
- Reduce maintenance burden when mocks need updating
- Ensure consistency across test suites
- Make individual spec files more focused on their actual tests
v1/src/simulator/spec/bitConvertor.spec.js (1)
112-116: Removeconsole.logfrom test.The
console.logstatement adds noise to test output, especially with--silentmode in the test script. If debugging info is needed, consider using Vitest's built-in logging or removing it for CI.Remove console.log
test('function setBaseValues working', () => { const randomBaseValue = Math.floor(Math.random() * 100) - console.log('Testing for Base Value --> ', randomBaseValue) expect(() => setBaseValues(randomBaseValue)).not.toThrow() })v1/src/simulator/spec/sequential.spec.js (1)
15-100: Consolidate shared test harness & confirmsetup()completionThe mocks + mount boilerplate are repeated across multiple spec files; consider extracting a shared helper to reduce drift. Also, please confirm whether
setup()performs async work—if it does, await it inbeforeAllto avoid partial initialization during tests.v1/src/simulator/spec/data.spec.js (1)
113-160: Reduce order-dependence across testsSeveral tests assume state produced by earlier tests (e.g.,
load(...)beforescheduleBackup,undo,redo). Consider isolating state per test (e.g.,beforeEachwith a freshload/reset) to avoid flaky behavior if sequencing changes.src/simulator/spec/gates.spec.js (1)
15-99: Consider extracting the shared test harness into a helper.
The mock/mount boilerplate here is repeated across multiple spec files; a shared helper (or centralized Vitest setup) would reduce duplication and keep mocks consistent.
- Add ignorePatterns for config files in eslint - Add vitest.workspace.ts to tsconfig.node.json - Fix vueI18n, path aliases for 'src' workspace - Refactor vitest.workspace.ts to use factory function
senutpal
force-pushed
the
refactor/test-workspace-configuration
branch
from
93ad29e to
5671610
Compare
|
Could you add this to CI as well so that we can validate on each pull request ? |
- Run tests on push to main and all pull requests - Use Node.js 22 with npm caching for performance - Enable concurrency control to cancel duplicate runs - Add github-actions reporter for inline PR annotations - Set 10-minute timeout for test execution
|
done @tachyons |
|
@senutpal Tests are red, could you add the fixes for the tests in this PR so that we can merge this with green CI |
|
Also v0 is symlinked to src. So there is no need to run tests for that dir separately |
|
all done @tachyons |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/simulator/spec/bitConvertor.spec.js`:
- Line 52: The test suite describe block currently reads 'data dir working'
which is a copy-paste error; update the describe block in
src/simulator/spec/bitConvertor.spec.js to a correct name reflecting the tests
(e.g., "bitConvertor utilities" or "BitConvertor") so the suite matches the file
purpose and tests for the bit converter functions; locate the describe(...) at
the top of the file and replace the string accordingly.
In `@src/simulator/spec/data.spec.js`:
- Around line 200-205: The test "OpenOffline working" uses optional chaining on
DOM selections which can mask missing elements; update the test around
openOffline() to explicitly query and assert the presence of the elements (e.g.,
the '#openProjectDialog input' element and the '#Open_offline_btn' element)
before calling click() so failures surface—locate the test function named
"OpenOffline working" and the openOffline() call, replace or augment the lines
with explicit queries and expect/assert statements that the elements are not
null (or throw) and only then call element.click(), then keep the final
expectation on globalScope.id.
♻️ Duplicate comments (4)
src/simulator/spec/vitestSetup.ts (1)
36-42: Consolidate teardown so suppression applies during cleanup.The first
afterAllruns beforeisTearingDownis set, so teardown errors won’t be suppressed; cleanup is also duplicated. Merge into a single teardown whereisTearingDownis set before cleanup and handlers are removed afterward.🛠️ Suggested consolidation
-// Restore real timers and cleanup after all tests -afterAll(() => { - vi.clearAllTimers() - vi.useRealTimers() - vi.clearAllMocks() - vi.restoreAllMocks() -}) +// Restore real timers and cleanup after all tests; suppress expected teardown errors +afterAll(() => { + isTearingDown = true + vi.clearAllTimers() + vi.useRealTimers() + vi.clearAllMocks() + vi.restoreAllMocks() + process.off('unhandledRejection', handleUnhandledRejection) + process.off('uncaughtException', handleUncaughtException) +}) ... -// Unregister error handlers and suppress expected teardown errors -afterAll(() => { - isTearingDown = true - process.off('unhandledRejection', handleUnhandledRejection) - process.off('uncaughtException', handleUncaughtException) - vi.clearAllTimers() - vi.useRealTimers() - vi.clearAllMocks() - vi.restoreAllMocks() -})Also applies to: 92-99
v1/src/simulator/spec/vitestSetup.ts (1)
36-42: Keep handlers registered while teardown cleanup runs.
isTearingDownis set, but handlers are removed before cleanup, so suppression can’t catch teardown errors. Reorder cleanup ahead ofprocess.off(...).🛠️ Suggested reorder
afterAll(() => { isTearingDown = true - process.off('unhandledRejection', handleUnhandledRejection) - process.off('uncaughtException', handleUncaughtException) vi.clearAllTimers() vi.useRealTimers() vi.clearAllMocks() vi.restoreAllMocks() + process.off('unhandledRejection', handleUnhandledRejection) + process.off('uncaughtException', handleUncaughtException) })Also applies to: 92-99
v1/src/simulator/spec/bitConvertor.spec.js (1)
52-52: Rename the suite to match the BitConvertor tests.✏️ Suggested rename
-describe('data dir working', () => { +describe('BitConvertor Testing', () => {v1/src/simulator/spec/combinationalAnalysis.spec.js (1)
115-120: Avoid mutating shared test fixtures (clone before edits).Line 115-119 mutates
testData.AndGatein-place, which can leak into other specs that import the same JSON and make tests order-dependent. Clone before editing and pass the clone intorunAll.♻️ Proposed fix
- testData.AndGate.groups[0].inputs[0].label = 'A' - testData.AndGate.groups[0].inputs[1].label = 'B' - testData.AndGate.groups[0].outputs[0].label = 'AB' - - const result = runAll(testData.AndGate) + const andGate = JSON.parse(JSON.stringify(testData.AndGate)) + andGate.groups[0].inputs[0].label = 'A' + andGate.groups[0].inputs[1].label = 'B' + andGate.groups[0].outputs[0].label = 'AB' + + const result = runAll(andGate)
🧹 Nitpick comments (12)
v1/src/simulator/spec/sequential.spec.js (1)
51-100: Add teardown to unmount the simulator and remove the mount node.This avoids DOM leakage across test files when isolation is disabled or shared workers are used.
♻️ Suggested teardown
describe('Simulator Sequential Element Testing', () => { let pinia let router + let wrapper + let mountEl @@ - const elem = document.createElement('div') + const elem = document.createElement('div') + mountEl = elem @@ - mount(simulator, { + wrapper = mount(simulator, { global: { plugins: [pinia, router, i18n, vuetify], }, attachTo: elem, }) @@ setup() }) + + afterAll(() => { + wrapper?.unmount() + mountEl?.remove() + })v1/src/simulator/spec/decoders-plexers.spec.js (1)
1-100: Consider extracting shared test setup/mocks into a helper.The setup here mirrors multiple spec files; a shared helper would reduce duplication and keep fixes in sync.
v1/src/simulator/spec/gates.spec.js (1)
15-49: Consider extracting shared test scaffolding to reduce duplication.This mock setup block (tauri, ResizeObserver, canvas context, codemirror) is duplicated nearly verbatim across all spec files in the PR. Extract these mocks to a shared setup file (e.g.,
vitestSetup.ts) and configure it in your Vitest workspace to run automatically.Note: The canvas context mock here is missing methods that
data.spec.jsincludes (e.g.,rect,save,restore,translate,scale,measureText,setLineDash,getImageData,rotate,putImageData,drawImage,bezierCurveTo). If any test indirectly triggers these methods, it will fail with "not a function" errors.src/simulator/spec/data.spec.js (1)
36-64: This canvas mock is the most comprehensive—use it as the shared template.This file includes additional canvas context methods (
rect,save,restore,translate,scale,measureText,setLineDash,getImageData,rotate,putImageData,drawImage,bezierCurveTo) that other spec files lack. When consolidating to a shared setup, use this version to avoid "method is not a function" errors in tests that trigger these canvas operations.src/simulator/spec/bitConvertor.spec.js (1)
112-116: Removeconsole.logfrom test.The
console.logstatement adds noise to test output. If you need to debug test values, use Vitest's verbose mode or debugging tools instead.♻️ Suggested fix
test('function setBaseValues working', () => { const randomBaseValue = Math.floor(Math.random() * 100) - console.log('Testing for Base Value --> ', randomBaseValue) expect(() => setBaseValues(randomBaseValue)).not.toThrow() })src/simulator/spec/sequential.spec.js (1)
15-49: Shared mock scaffolding duplicated here too.Same duplication concern as noted for other spec files. The canvas mock here uses the basic set of methods, which may be insufficient if sequential element tests indirectly trigger additional canvas operations.
src/simulator/spec/subCircuit.spec.js (1)
15-49: Same mock scaffolding duplication.As with other spec files, this boilerplate should be consolidated into a shared setup file.
v1/src/simulator/spec/data.spec.js (2)
26-76: Significant mock duplication across test files—consider extracting to a shared setup module.This file and the other spec files (misc.spec.js, complexCircuit.spec.js, combinationalAnalysis.spec.js, decoders-plexers.spec.js) all contain nearly identical mock code (~90 lines) for tauri events, ResizeObserver, HTMLCanvasElement.getContext, and codemirror modules. This v1 version has the most comprehensive canvas mock with additional methods (e.g.,
getSerializedSvg,getImageData,drawImage,bezierCurveTo).Extract these mocks to a shared test utility file (e.g.,
testSetupMocks.ts) and import it in each spec. This reduces maintenance burden and ensures consistency across test files.
200-205: Potential brittleness in DOM selector assertions.The test relies on specific DOM selectors (
#openProjectDialog input,#Open_offline_btn) which may break if the component structure changes. Consider adding data-testid attributes to the component for more robust test selectors.src/simulator/spec/complexCircuit.spec.js (1)
53-53: Misleading describe block name—should reflect complex circuit testing.The describe block is named
'data dir working'but this file tests complex circuits (ripple carry adder, ALU). Consider renaming to something more descriptive like'Complex Circuit Testing'or'Simulator Complex Circuit Testing'.Suggested fix
-describe('data dir working', () => { +describe('Simulator Complex Circuit Testing', () => {src/simulator/spec/combinationalAnalysis.spec.js (1)
114-121: Test mutates shared testData object—could cause test pollution.Lines 115-117 directly mutate
testData.AndGate.groups[0].inputs[0].label, etc. If this testData object is reused in other tests (within this file or across the test suite), these mutations persist and may cause unexpected behavior or flaky tests.Consider deep-cloning the test data before mutation:
Suggested fix
test('testing Combinational circuit', () => { - testData.AndGate.groups[0].inputs[0].label = 'A' - testData.AndGate.groups[0].inputs[1].label = 'B' - testData.AndGate.groups[0].outputs[0].label = 'AB' + const andGateData = structuredClone(testData.AndGate) + andGateData.groups[0].inputs[0].label = 'A' + andGateData.groups[0].inputs[1].label = 'B' + andGateData.groups[0].outputs[0].label = 'AB' - const result = runAll(testData.AndGate) + const result = runAll(andGateData) expect(result.summary.passed).toBe(3) })src/simulator/spec/decoders-plexers.spec.js (1)
1-100: Consistent test scaffolding—duplicated setup should be centralized.This file follows the same pattern as other spec files with identical mocks and setup code. As noted in the v1/data.spec.js review, this ~90 lines of duplicated code across all spec files should be extracted to a shared test utility module to improve maintainability.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
v1/src/simulator/src/node.js (1)
399-452: Switch toletfor loop variables inresolve().Two loops in the
resolve()function redeclarevar iin the same function scope, triggering Biome'snoRedeclarerule.letis block-scoped and will resolve this:🔧 Proposed fix
- for (var i = 0; i < this.connections.length; i++) { + for (let i = 0; i < this.connections.length; i++) { ... - for (var i = 0; i < this.connections.length; i++) { + for (let i = 0; i < this.connections.length; i++) { ...src/simulator/src/node.js (1)
399-452: Replacevarwithletinresolve()loops to avoid redeclaration.The
resolve()method declaresvar itwice in the same function scope (lines 400 and 450), which violates Biome'snoRedeclarerule. Sincevaris function-scoped, both loop variables collide. Switching toletcreates block scope for each loop.🔧 Proposed fix
- for (var i = 0; i < this.connections.length; i++) { + for (let i = 0; i < this.connections.length; i++) { ... - for (var i = 0; i < this.connections.length; i++) { + for (let i = 0; i < this.connections.length; i++) { ...
♻️ Duplicate comments (2)
v1/src/simulator/src/node.js (1)
454-520: Add explicit fallthrough annotations for Biome.Biome flags these intentional fallthroughs. Add explicit
biome-ignore(or the accepted fallthrough marker) so CI doesn’t fail on lint.🧹 Suggested suppression pattern
- case NODE_OUTPUT: + // biome-ignore lint/suspicious/noFallthroughSwitchClause + case NODE_OUTPUT: ... - case NODE_INPUT: + // biome-ignore lint/suspicious/noFallthroughSwitchClause + case NODE_INPUT: ... - case NODE_INTERMEDIATE: + // biome-ignore lint/suspicious/noFallthroughSwitchClause + case NODE_INTERMEDIATE:src/simulator/src/node.js (1)
454-520: Add explicit fallthrough annotations for Biome.Biome flags these intentional fallthroughs. Add explicit
biome-ignore(or the accepted fallthrough marker) so CI doesn’t fail on lint.🧹 Suggested suppression pattern
- case NODE_OUTPUT: + // biome-ignore lint/suspicious/noFallthroughSwitchClause + case NODE_OUTPUT: ... - case NODE_INPUT: + // biome-ignore lint/suspicious/noFallthroughSwitchClause + case NODE_INPUT: ... - case NODE_INTERMEDIATE: + // biome-ignore lint/suspicious/noFallthroughSwitchClause + case NODE_INTERMEDIATE:
🧹 Nitpick comments (1)
src/simulator/src/node.js (1)
125-131: Guard global constant exposure for non‑browser runtimes.Top‑level
window.*assignments will throw if this module is imported under a Node/Vitestenvironment: "node"or SSR. Consider usingglobalThisor a guarded block.🛠️ Proposed refactor
- window.NODE_INPUT = NODE_INPUT - window.NODE_OUTPUT = NODE_OUTPUT - window.NODE_INTERMEDIATE = NODE_INTERMEDIATE + const globalTarget = typeof window !== 'undefined' ? window : globalThis + globalTarget.NODE_INPUT = NODE_INPUT + globalTarget.NODE_OUTPUT = NODE_OUTPUT + globalTarget.NODE_INTERMEDIATE = NODE_INTERMEDIATE
|
@tachyons ci is green now |
| let pinia; | ||
| let router; |
There was a problem hiding this comment.
@senutpal Could you revert all unrelated changes including semicolon removal. Because it makes PR too big
There was a problem hiding this comment.
@tachyons my editor auto-formats on save because .prettierrc has semi: false, so it keeps undoing the fixes i make. it’s getting pretty hectic to manually revert those changes. is there any git workflow to separate formatting changes from actual logic changes, or some interactive/diff-based way to undo only the formatting ?
There was a problem hiding this comment.
@tachyons i am thinking to open a new clean pr what do you say ?
| for (var i = 0; i < this.parent.scope.wires.length; i++) { | ||
| if (this.parent.scope.wires[i].checkConvergence(this)) { | ||
| if ( | ||
| this.parent.scope.wires[i].node1 === this || |
3 participants
Fixes #696 #697 #724 #772
Summary
Refactored test configuration from single-project to multi-project workspace supporting three isolated test environments (
src,v0,v1).Changes Made
1. New File:
vitest.workspace.tsCreated multi-project workspace configuration with three isolated projects:
fileURLToPath(import.meta.url)andpath.join()for cross-platform path resolution2. Modified:
package.jsonUpdated test scripts:
--workspace vitest.workspace.tsto use new configuration--silentfor cleaner output3. Modified:
vite.config.tsRemoved inline test configuration (lines 63-74):
When using
--workspaceflag, Vitest reads config from workspace file only. Inline test config would cause conflicts and duplicate test execution.Rationale
The codebase has three parallel implementations. Each needs isolated test environments because:
#resolves differently)When using
--workspace, Vitest ignores inline test config. Keeping it causes:The
extends: './vite.config.ts'in workspace projects preserves non-test configuration (plugins, build options).Screenshots -
Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?
If you used AI assistance:
Explain your implementation approach:
The workspace configuration was designed to maintain backward compatibility with existing tests while adding isolation. Each project
extendsthe mainvite.config.tsto inherit plugins and build settings, but overridesresolve.aliasandtest.setupFilesfor project-specific behavior. The dynamic path resolution usingfileURLToPathandpath.joinensures cross-platform compatibility without hardcoded paths.Checklist before requesting a review
Note: Please check Allow edits from maintainers if you would like us to assist in the PR.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.