-
Notifications
You must be signed in to change notification settings - Fork 16
test: introduce test-vitest-setup #1067
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@code-pushup/ci
@code-pushup/cli
@code-pushup/create-cli
@code-pushup/core
@code-pushup/models
@code-pushup/nx-plugin
@code-pushup/coverage-plugin
@code-pushup/eslint-plugin
@code-pushup/js-packages-plugin
@code-pushup/jsdocs-plugin
@code-pushup/lighthouse-plugin
@code-pushup/typescript-plugin
@code-pushup/utils
@code-pushup/models-transformers
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx for starting this refactor! Looking forward to a clean test config setup.
Some remarks, If you open a PR and request a review please do a self review.
Check if:
- all changes are applied in a consistent way. Not different versions for the same change. E.g. you extend the global configuration in different ways.
- do not add code changes out of scope. If they are necessary mention them in a comment
- potentially add information on the state of the PR e.g.
DO NOT REVIEWand additional comments about unfinished parts. - If you use AI do an extra detailed review!
|
View your CI Pipeline Execution ↗ for commit 39f4030
☁️ Nx Cloud last updated this comment at |
| type CoverageConfig = { | ||
| enabled?: boolean; | ||
| provider: 'v8'; | ||
| reporter: ('text' | 'lcov')[]; | ||
| reportsDirectory: string; | ||
| include: string[]; | ||
| exclude: string[]; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use the types from vite/vitest directly:
// from vite
import type { UserConfig } from 'vite';
type TestCoverage = Exclude<UserConfig['test'], undefined>;
// from vitest
import type { UserConfig } from 'vitest';
type TestCoverage = UserConfig;
// for both
const testConfig: TestCoverage = {
coverage: {
reporter: ['text', 'lcov'],
reportsDirectory: '../../coverage/plugin-eslint/int-tests',
exclude: ['mocks/**', '**/types.ts'],
},
};I suggest go with types from vitest and remove the types in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw you removed the custom types. Thanks!
I also saw you just do not us types now in your helpers.
Please use the types in:
getDefaultTestSettingsreturn typeTestCoverage['coverage']fromimport type { UserConfig } from 'vitest'createSharedVitestConfigreturn typeUserConfigfromimport type { UserConfig } from 'vitest'createSharedUnitVitestConfigreturn typeUserConfigfromimport type { UserConfig } from 'vitest'createSharedIntegrationVitestConfigreturn typeUserConfigfromimport type { UserConfig } from 'vitest'createSharedE2eVitestConfigreturn typeUserConfigfromimport type { UserConfig } from 'vitest'- ... also look at other places that could improve typing
|
|
||
| it('should return pass if no files are given and pass', async () => { | ||
| vol.reset(); | ||
| // create empty directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is out of scope and not needed- revert pls.
| vol.fromJSON( | ||
| { | ||
| 'e2e/package.json': pkgJsonContent({ name: 'e2e' }), | ||
| 'package.json': pkgJsonContent({ name: 'example-monorepo' }), // not in patterns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is out of scope and not needed. Please revert.
| workspaces: ['ui', 'api'], | ||
| }), | ||
| 'api/package.json': pkgJsonContent({ name: 'api' }), | ||
| 'e2e/package.json': pkgJsonContent({ name: 'e2e' }), // not in workspaces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is out of scope and not needed. Please revert.
| }), | ||
| ); | ||
|
|
||
| // values come from CORE_CONFIG_MOCK returned by readRcByPath mock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is out of scope and not needed. Please revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thanks for resolving some of the comments!
For next steps and to get this in faster i suggest the following:
- please add a proper PR description that contains the scope and planned changes
- do not add code changes out of scope. If they are necessary mention them in a comment.
This happened the 3rd time in a row now and makes it impossible to merge. This time you did remove the AI comments as mentioned, but you also removed comments all over the code base. I mentioned some of the places in comments. - If you use AI do an extra detailed review! This was the case in all PR's so far and made the review a bit more harder/longer, as the changes needs to get pointed out individually and reverted again. By doing a self review those D-tours could be avoided or at least reduced.
- For the PR description that defines the scope I suggest the following:
This PR includes:
- Introduce
test-vitest-setupand add configuration
- adjust coverage.reportDirectory to be at project root (
packages/<name>/.coverage)Related:
-
With this scope I would suggest revert all changed other than the folder
testing/test-vitest-setup -
The comment regarding typing got updated and should serve now also the places where you can use the types. This should clear any misunderstanding. If there is something unclear regarding the types please let me know.
Comment: #1067 (comment)
|
Close as reopened in other PRs. |
Closes #1065