workflow to run all specs in src/simulator/spec
#510
Conversation
WalkthroughA new GitHub Actions workflow for simulator tests has been added. This workflow is triggered on pushes and pull requests to the Changes
Sequence Diagram(s)sequenceDiagram
participant G as GitHub
participant W as Workflow Runner
participant C as Checkout Action
participant N as Node Setup Action
participant I as Install Dependencies
participant T as Test Executor
G->>W: Trigger workflow (push/PR to main)
W->>C: Run checkout action
C-->>W: Repository checked out
W->>N: Setup Node.js v20
N-->>W: Node environment ready
W->>I: Execute 'npm install'
I-->>W: Dependencies installed
W->>T: Run simulator tests (npm run test -- src/simulator/spec)
T-->>W: Test results returned
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 (1)
🪧 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 (
|
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@vedant-jain03 @niladrix719 @aryanndwi123 created workflow for spec files in src/simulator/spec. |
![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 (1)
.github/workflows/simulator-tests.yml (1)
26-28: Dependency Installation Command
The workflow usesnpm installto install dependencies. If apackage-lock.jsonor similar exists and reproducibility is critical, consider usingnpm cifor a cleaner, reproducible build environment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/simulator-tests.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
.github/workflows/simulator-tests.yml (6)
1-2: Workflow Naming is Clear
The workflow name "Simulator Tests" clearly conveys its purpose and aligns with the PR objectives.
3-12: Trigger Event Configuration
The workflow is configured to trigger on both push and pull_request events for the "main" and "dev" branches. This setup meets the project requirements to run tests on relevant branches.
13-16: Job Configuration is Appropriate
The job "run-simulator-tests" is set to run onubuntu-latest, which is a suitable environment for this workflow.
17-20: Repository Checkout Step
The "Checkout Repository" step correctly usesactions/checkout@v4to ensure that the source code is available for the test run.
21-25: Node.js Setup Step
The Node.js setup is defined withactions/setup-node@v4and setsnode-version: 22as specified. Confirm that Node.js 22 is indeed the legacy version consistently used across the project.
29-31: Simulator Test Execution Command
The commandnpm run test -- src/simulator/speccorrectly targets the directory where the simulator specifications reside. Ensure that the test script inpackage.jsonsupports receiving the additional argument and executes the intended tests.
|
The tests are failing to even start |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/components/Extra.vue (1)
258-268:⚠️ Potential issueFix TypeScript type annotation in event handler
There's a parsing error in the event handler due to the use of React.MouseEvent type in a Vue component. Update to use TypeScript's native event type.
- @mousedown="(e: React.MouseEvent) => { + @mousedown="(e: MouseEvent) => { if(simulationArea.shiftDown == false) { simulationArea.shiftDown = true; selectMultiple = true; } else { simulationArea.shiftDown = false; selectMultiple = false; e.preventDefault(); } }"🧰 Tools
🪛 ESLint
[error] 258-258: Parsing error: Unexpected token :.
(vue/no-parsing-error)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
src/components/Extra.vue(10 hunks)src/components/Navbar/Navbar.vue(2 hunks)src/components/Navbar/QuickButton/QuickButtonMobile.vue(2 hunks)src/components/Panels/ElementsPanel/ElementsPanelMobile.vue(5 hunks)src/components/Panels/PropertiesPanel/ModuleProperty/ProjectProperty/ProjectProperty.vue(1 hunks)src/components/Panels/PropertiesPanel/PropertiesPanelMobile.vue(1 hunks)src/components/Panels/TimingDiagramPanel/TimingDiagramButtons.vue(1 hunks)src/components/Panels/TimingDiagramPanel/TimingDiagramMobile.vue(1 hunks)src/components/Panels/VerilogEditorPanel/VerilogEditorPanelMobile.vue(1 hunks)src/components/ReportIssue/ReportIssueButton.vue(1 hunks)src/pages/simulatorHandler.vue(1 hunks)src/simulator/src/Verilog2CV.js(1 hunks)src/simulator/src/circuit.ts(1 hunks)src/simulator/src/layoutMode.ts(1 hunks)src/simulator/src/listeners.js(1 hunks)src/simulator/src/plotArea.js(1 hunks)src/simulator/src/ux.js(1 hunks)
✅ Files skipped from review due to trivial changes (12)
- src/simulator/src/circuit.ts
- src/components/ReportIssue/ReportIssueButton.vue
- src/pages/simulatorHandler.vue
- src/simulator/src/Verilog2CV.js
- src/components/Panels/VerilogEditorPanel/VerilogEditorPanelMobile.vue
- src/components/Panels/PropertiesPanel/ModuleProperty/ProjectProperty/ProjectProperty.vue
- src/components/Panels/TimingDiagramPanel/TimingDiagramButtons.vue
- src/simulator/src/layoutMode.ts
- src/simulator/src/ux.js
- src/components/Panels/PropertiesPanel/PropertiesPanelMobile.vue
- src/simulator/src/listeners.js
- src/components/Navbar/Navbar.vue
🧰 Additional context used
🪛 ESLint
src/components/Extra.vue
[error] 47-51: Elements in iteration expect to have 'v-bind:key' directives.
(vue/require-v-for-key)
[error] 54-58: Elements in iteration expect to have 'v-bind:key' directives.
(vue/require-v-for-key)
[error] 258-258: Parsing error: Unexpected token :.
(vue/no-parsing-error)
🔇 Additional comments (10)
src/components/Panels/TimingDiagramPanel/TimingDiagramMobile.vue (1)
37-37: Import path updated to use relative pathThe import path for
useSimulatorMobileStorehas been changed from an absolute path using the alias prefix#/to a relative path. This change appears to be part of a consistent effort across multiple files to standardize import paths.src/simulator/src/plotArea.js (1)
6-6: Import path updated to use relative pathThe import path for
useSimulatorMobileStorehas been changed from an absolute path using the alias prefix#/to a relative path. This change is consistent with similar updates across multiple files.src/components/Navbar/QuickButton/QuickButtonMobile.vue (2)
2-123: Template reformatting and UI enhancements for mobile viewThe template has been reformatted with improved indentation and expanded multi-line attributes for better readability. Two significant UI enhancements have been added:
- A hamburger menu navigation for mobile users (lines 97-102)
- A zoom slider with increment/decrement controls (lines 104-122)
These changes improve the mobile user experience while maintaining existing functionality.
130-130: Import path updated to use relative pathThe import path for
useSimulatorMobileStorehas been changed from an absolute path using the alias prefix#/to a relative path. This change is consistent with similar updates across multiple files.src/components/Extra.vue (1)
378-378: Import path updated to use relative pathThe import path for
useSimulatorMobileStorehas been changed from an absolute path using the alias prefix#/to a relative path. This change is consistent with similar updates across multiple files.src/components/Panels/ElementsPanel/ElementsPanelMobile.vue (5)
2-148: Improved template formattingThe template section has been reformatted with consistent indentation and spacing, making the component structure more readable while maintaining the same functionality.
200-236: Enhanced watch functions readabilityThe watch functions have been reformatted with improved indentation and structure, making the code easier to read and maintain.
302-314: Improved type safety with ElementsTypeThe selectCategory function now uses an explicit ElementsType parameter with a default value, which improves type safety.
325-424: Consistent style section formattingThe style section has been reformatted with consistent indentation and the CSS selectors for scrollbar styling have been logically grouped, improving readability.
158-158:✅ Verification successful
Import path change from absolute to relative
The import path for
useSimulatorMobileStorehas been changed from an absolute path (likely#/store/simulatorMobileStore) to a relative path. This appears to be part of a broader effort to standardize import paths across the project as mentioned in the PR summary.Make sure this change is consistent with the project's conventions for import paths.
🏁 Script executed:
#!/bin/bash # Check for consistency in import path patterns for this store across the project rg -l "useSimulatorMobileStore" --type vue | xargs grep -l "from.*store/simulatorMobileStore"Length of output: 121
Import Path Change Consistency – Manual Verification Required
- The
useSimulatorMobileStoreimport insrc/components/Panels/ElementsPanel/ElementsPanelMobile.vuehas been updated from an absolute path (likely#/store/simulatorMobileStore) to a relative path (../../store/simulatorMobileStore), aligning with the PR’s effort to standardize import paths.- Our initial automated check using the file type filter (
--type vue) resulted in an error (unrecognized file type: vue). This prevents us from conclusively verifying that all.vuefiles use the correct import pattern.- To further verify consistency, you might run an alternative command that explicitly targets Vue files. For example:
Please run this command or manually check that all occurrences of the store import adhere to the project conventions.
🏁 Script executed:
#!/bin/bash # Re-verify by explicitly searching through .vue files using a glob pattern rg -g '*.vue' -l "useSimulatorMobileStore" | xargs grep -l "from.*store/simulatorMobileStore"Length of output: 823
Import Path Update Verified
- The updated import in
src/components/Panels/ElementsPanel/ElementsPanelMobile.vuenow correctly uses a relative path (../../store/simulatorMobileStore).- A revised search targeting
.vuefiles returned multiple files—includingElementsPanelMobile.vue—that referencestore/simulatorMobileStore, which confirms that the project's switch to relative import paths is consistently applied.- Please manually confirm that none of these files still use the outdated absolute alias (e.g.
#/store/simulatorMobileStore).
| <div | ||
| v-for="error in useState().errorMessages" | ||
| class="alert alert-danger" | ||
| role="alert" | ||
| > | ||
| {{ error }} | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add key directive to v-for elements
Elements in v-for iterations need to have a unique key for Vue's rendering optimization.
<div
v-for="error in useState().errorMessages"
+ :key="error"
class="alert alert-danger"
role="alert"
>
{{ error }}
</div>📝 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.
| <div | |
| v-for="error in useState().errorMessages" | |
| class="alert alert-danger" | |
| role="alert" | |
| > | |
| {{ error }} | |
| </div> | |
| <div | |
| v-for="error in useState().errorMessages" | |
| :key="error" | |
| class="alert alert-danger" | |
| role="alert" | |
| > | |
| {{ error }} | |
| </div> |
🧰 Tools
🪛 ESLint
[error] 54-58: Elements in iteration expect to have 'v-bind:key' directives.
(vue/require-v-for-key)
| <div | ||
| v-for="mes in useState().successMessages" | ||
| class="alert alert-success" | ||
| role="alert" | ||
| > | ||
| {{ mes }} | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add key directive to v-for elements
Elements in v-for iterations need to have a unique key for Vue's rendering optimization.
<div
v-for="mes in useState().successMessages"
+ :key="mes"
class="alert alert-success"
role="alert"
>
{{ mes }}
</div>📝 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.
| <div | |
| v-for="mes in useState().successMessages" | |
| class="alert alert-success" | |
| role="alert" | |
| > | |
| {{ mes }} | |
| </div> | |
| <div | |
| v-for="mes in useState().successMessages" | |
| :key="mes" | |
| class="alert alert-success" | |
| role="alert" | |
| > | |
| {{ mes }} | |
| </div> |
🧰 Tools
🪛 ESLint
[error] 47-51: Elements in iteration expect to have 'v-bind:key' directives.
(vue/require-v-for-key)
|
@niladrix719 i fixed the prev errors coming while running the workflow. but i think errors are due to the tests fails. PTAL! |
|
try ignoring node_modules and v1/ v0/ dir for now, in the test config |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/simulator-tests.yml (2)
21-25: Node.js Setup Version Mismatch
The step uses Node.js version 20 (node-version: 20), but the PR objective mentions setting up Node.js version 22 to align with the legacy version used in the project. Please verify that the intended Node.js version is correctly specified. If Node.js version 22 is required, consider updating this configuration.- node-version: 20 + node-version: 22
29-31: Test Command Configuration and Directory Exclusions
The commandnpm run test -- src/simulator/specappears to be correct for running tests in the specified directory. However, given earlier comments regarding failing tests and the suggestion to ignore directories such asnode_modules,v1/, andv0/, please ensure that the test runner configuration (for example, via a configuration file like Jest’stestPathIgnorePatterns) accommodates these exclusions. This will help prevent interference from non-test files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
.github/workflows/simulator-tests.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
.github/workflows/simulator-tests.yml (5)
1-2: Workflow Name is Appropriately Defined
The workflow is named "Simulator Tests," which clearly reflects the intended purpose.
3-11: Workflow Triggers are Properly Configured
The workflow is set to trigger on both push and pull request events on the main branch. This is standard for ensuring CI runs on critical branches.
13-16: Job Definition is Correct
The job "run-simulator-tests" is correctly defined to run on the Ubuntu-latest runner. This setup aligns with typical CI environments.
17-20: Repository Checkout Step is Standard
Usingactions/checkout@v4ensures that the repository is properly cloned, which is a best practice for workflows.
26-28: Dependency Installation Step is Correct
Runningnpm installis the standard command to install dependencies. No changes are needed here.
|
@gitsofaryan Could you please try and run the specs and make sure everything passes ? |
|
Running this cmd |
There was a problem hiding this comment.
You have done a lot of things in the PR and a few of them make it unprofessional (I am sure you can improve them from next PRs ) :
-
You have changed the paths from using aliases to a relative path , you should not do this since it breaks the continuity of the code, I am guessing you did this bcoz the tests were failing. Well that is bcoz of inconsistency of the alias declaration in the normal environment and runtime environment, so change that before running tests.
-
You shoudnt make changes in yml and css etc in the same PR.
Your PR #515 is enough and you should close this one
Fixes #508
Describe the changes you have made in this PR -
Screenshots of the changes (If any) -
Working
Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.
Summary by CodeRabbit
useSimulatorMobileStoreto relative paths across multiple files for better module resolution.