chore: add script to collect coverage with atomics support to speed up…#495
Merged
ngundotra merged 2 commits intosolana-foundation:vitestfrom Mar 17, 2025
Merged
chore: add script to collect coverage with atomics support to speed up…#495ngundotra merged 2 commits intosolana-foundation:vitestfrom
ngundotra merged 2 commits intosolana-foundation:vitestfrom
Conversation
|
@rogaldh is attempting to deploy a commit to the Solana Foundation Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to b4347bb in 2 minutes and 45 seconds
More details
- Looked at
43lines of code in2files - Skipped
1files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. vite.config.ts:35
- Draft comment:
Ensure 'poolOptions.threads.useAtomics' is supported by your Vitest version. If so, consider adding a comment describing its impact. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%<= threshold50%
The comment is asking the author to ensure compatibility with a specific version of a tool, which is not allowed. However, it also suggests adding a comment to describe the impact, which could be useful. The first part of the comment violates the rules, but the second part is a valid suggestion.
2. package.json:12
- Draft comment:
The CLI flag '--coverage.enabled' looks unusual. Consider using '--coverage' (e.g. 'vitest run --coverage') to trigger coverage collection. - Reason this comment was not posted:
Marked as duplicate.
3. package.json:93
- Draft comment:
Ensure that '@vitest/coverage-v8@3.0.8' is fully compatible with the installed Vitest (^3.0.8). - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is related to dependency compatibility, which is not allowed according to the rules. It asks the author to ensure compatibility, which is similar to asking them to double-check something.
4. package.json:97
- Draft comment:
Verify that 'jsdom@^26.0.0' is the intended version and works well with the testing environment (e.g. 'jest-environment-jsdom' at 29.5.0). - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to verify compatibility between a specific version of a library and the testing environment. This falls under the rule of not asking the author to ensure compatibility or verify intentions, which is not allowed.
5. vite.config.ts:35
- Draft comment:
Consider adding a comment explaining why 'poolOptions' with 'threads.useAtomics: true' is enabled, as it improves performance but may require fallback handling on unsupported systems. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment suggests adding an explanation for a specific configuration choice, which is not allowed by the rules. It doesn't provide a specific code suggestion or ask for a test to be written. It also doesn't point out a potential issue or ask for confirmation of an intentional change. Therefore, it should be removed.
6. package.json:12
- Draft comment:
It appears there's a typographical error in the new 'coverage' script. The command is written as 'vitest --coverage.enabled', but typically the coverage flag should be '--coverage'. Please review if this is intentional or a mistake. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The comment is suggesting a change that would actually break the coverage command. Vitest's coverage configuration uses dot notation, and--coverage.enabledis the correct syntax. This is especially clear since they've also added the @vitest/coverage-v8 package as a dependency, showing this was an intentional setup.
Could there be multiple valid ways to enable coverage in Vitest? Maybe the simpler --coverage flag also works?
Even if --coverage might work, the current syntax is definitely valid and intentionally specific. We shouldn't suggest changing working, correct code.
The comment should be deleted because it's suggesting to change correct, working code to a different syntax without any benefit.
Workflow ID: wflow_xht2mRmIXcPTSfwn
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
ngundotra
pushed a commit
that referenced
this pull request
Mar 17, 2025
…p… (#495) ## Description Add script to collect coverage ## Type of change - [x] Other (please describe): add coverage script ## Related Issues Relates to #494 ## Checklist - [x] My code follows the project's style guidelines - [ ] I have added tests that prove my fix/feature works - [x] All tests pass locally and in CI - [ ] I have updated documentation as needed - [x] CI/CD checks pass - [ ] I have included screenshots for protocol screens (if applicable) - [ ] For security-related features, I have included links to related information <!-- ELLIPSIS_HIDDEN --> ---- > [!IMPORTANT] > Add script and configuration for test coverage collection with atomic support using Vitest. > > - **Scripts**: > - Add `coverage` script in `package.json` to run `vitest --coverage.enabled`. > - **Dependencies**: > - Add `@vitest/coverage-v8` and `jsdom` to `devDependencies` in `package.json`. > - **Configuration**: > - Update `vite.config.ts` to enable atomic support in `poolOptions.threads.useAtomics` for tests. > > <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=solana-foundation%2Fexplorer&utm_source=github&utm_medium=referral)<sup> for b4347bb. It will automatically update as commits are pushed.</sup> <!-- ELLIPSIS_HIDDEN --> --------- Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add script to collect coverage
Type of change
Related Issues
Relates to #494
Checklist
Important
Add script and configuration for test coverage collection with atomic support using Vitest.
coveragescript inpackage.jsonto runvitest --coverage.enabled.@vitest/coverage-v8andjsdomtodevDependenciesinpackage.json.vite.config.tsto enable atomic support inpoolOptions.threads.useAtomicsfor tests.This description was created by
for b4347bb. It will automatically update as commits are pushed.