refactor: common vitest configurations #1107
Conversation
github-actions
bot
added
📖 Project documentation
|
View your CI Pipeline Execution ↗ for commit 7008a48
☁️ Nx Cloud last updated this comment at |
@code-pushup/ci
@code-pushup/cli
@code-pushup/core
@code-pushup/create-cli
@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: |
Code PushUp🤨 Code PushUp report has both improvements and regressions – compared current commit 5a5fcfa with previous commit 33714e2. 🕵️ See full comparison in Code PushUp portal 🔍 🏷️ Categories
👎 3 groups regressed, 👍 4 audits improved, 👎 4 audits regressed, 14 audits changed without impacting score🗃️ Groups
18 other groups are unchanged. 🛡️ Audits
588 other audits are unchanged. |
| const testTimeout = | ||
| kind === 'unit' | ||
| ? TEST_TIMEOUTS.MEDIUM | ||
| : kind === 'int' | ||
| ? TEST_TIMEOUTS.LONG | ||
| : TEST_TIMEOUTS.E2E; |
There was a problem hiding this comment.
Let's avoid conditionals in tests. It makes the test code harder to read and adds noise.
| ['unit', setupPresets.unit.base, createUnitConfig], | ||
| ['int', setupPresets.int.base, createIntConfig], | ||
| ['e2e', setupPresets.e2e.base, createE2eConfig], | ||
| ] as const)('%s config creation', (kind, baseSetupFiles, createFn) => { |
There was a problem hiding this comment.
The each block for describe is introducing conditional checks in the it blocks as well as const. It further makes the tests harder to read. Let's avoid conditionals in tests as good as possible.
If you want to test basic values you could have a separate it block using the each for static things like defaults that are always the same. In the specific cases you can use expect.objectContaining to keep is focused.
| export function createVitestConfig( | ||
| options: VitestConfigFactoryOptions, | ||
| overrides: VitestOverrides = {}, | ||
| ): ViteUserConfig { |
There was a problem hiding this comment.
I suggest excluding overrides from the logic and push it to the user. This avoids complexity and reduces maintenance efforts to sync with vitest changes. WDYT @matejchalk
There was a problem hiding this comment.
Please let me know which direction we should follow:
- Easier override handling ( current solution ) - in this case we take care of most of the overrides but on the other hand we are exposed to vitest changes (if the happen) and we need to keep up it them
- We go easy way and move transfer the "burden" of overrides to end consumer / user.
Both approaches have pros and cons. I don't feel like I'm the right person to make a decision here 😅
There was a problem hiding this comment.
I think I've answered this comprehensively in my review. 😁
Unless there's a really good reason for projects to have something configured differently from others, I would hard-code it in the factory functions.
| projectKey: string; | ||
| kind: TestKind; | ||
| projectRootUrl: URL; | ||
| cacheDirName: string; |
There was a problem hiding this comment.
you could derive the cacheDirName from the projectKey. Internally if needed you can modify if needed e.g. cache-${projectKey}.
There was a problem hiding this comment.
Is it suggestion or request change 😄 ?
Deriving cacheDirName from projectKey in form: cache-${projectKey} sounds ok but makes me wonder, aren't we making some hidden bond between project key and cache directory?
Maybe we could consider cacheDirName as optional which fallbacks to cache-${projectKey} if not specified?
Let me know @BioPhoton
There was a problem hiding this comment.
EDIT:
We already using such bond for coverage, let's do the same for cache. I'll update it
There was a problem hiding this comment.
Maybe we could consider cacheDirName as optional which fallbacks to ...
Yes better!
cache-${projectKey} if not specified?
the "cache-" was just something to give example. In general the cache key (and directory) should be unique across the repository. That is the reason we are about it in the first place.
There was a problem hiding this comment.
Method you highlighted in comment is kind of "internal", the fallback is done in line 50
Is that ok acceptable or do you suggest other approach to that?
There was a problem hiding this comment.
I think your current implementation is unnecessarily flexible.
The only thing we care about is that the cache key is unique. Since Nx project names are unique, these can be used. But we don't need to override this per project.
| }, | ||
| { | ||
| test: { | ||
| testTimeout: 60_000, |
There was a problem hiding this comment.
this could be a default for the e2e target
There was a problem hiding this comment.
I checked other files and there are many different values, like 20k, 40k, 80k, 60k, 120k.
Just making sure that we want to have 60k as default?
There was a problem hiding this comment.
Hmmm.... I would put 20 as default. Or if there are more projects with 40 lets go with 40. And for the projects with higher or lower times go with a override.
Did not realise this is already an overwrite..
There was a problem hiding this comment.
I checked that and:
| Timeout | Occurences |
|---|---|
| 20s | 6 |
| 40s | 1 |
| 60s | 1 |
| 80s | 2 |
| 120s | 1 |
I suggest then use 20s as default (20_000ms)
There was a problem hiding this comment.
The timeouts might actually be a justifiable exception to keeping everything unified. 🤔 Some E2E tests require a generous test timeout, but it wouldn't be optimal to have a high test timeout everywhere.
I would handle these kinds of situations with a limited options object. For example:
- without custom options:
export default createIntTestConfig('utils');
- with custom options (custom
{ testTimeout?: number }type, not the full Vitest config)export default createIntTestConfig('utils', { testTimeout: 40_000 });
However, it would be worth considering if we really need to configure a custom timeout for a whole project. It's also possible to override timeouts per individual describe or it blocks. This may be a better targeted approach. We want to avoid higher timeouts if possible, but if they're not needed, it's best to know for which test precisely this is the case. Then we could have 3 fixed project-level timeouts for each kind of test (unit tests should be much faster than E2E tests, for example).
poltorak
force-pushed
the
refactor/vitest-configurations
branch
from
9433ca5 to
7008a48
Compare
There was a problem hiding this comment.
Hi, thanks for your contribution 🙂 I think it looks promising, but ...
I don't think this PR goes far enough towards unification. Ideally, only 2 parameters1 would determine the entire Vitest config:
- Nx project name (e.g.,
cli,plugin-eslint,ci-e2e) 2 - test kind (
'unit' | 'int' | 'e2e')
My main criticism of this PR is that it doesn't commit to unification over flexibility. I believe we need less flexibility than it may appear, so I would go all in on unification. If we find some exception, we should question whether that exception is justified and get rid of it if possible.
In the current implementation, some things are automatically inferred, but there are many (IMHO unnecessary) optional arguments, and anything can be overridden by providing a Vitest config object. To me, this is a leaky abstraction.
I've suggested specifics on what can be unified in my comments. Let me know if I can clarify anything 🙂 I realize you couldn't reasonably have known many of these things, so no worries.
Footnotes
-
By "parameter", I don't strictly mean a function parameter. Your approach of exporting different factory functions for each test kind also works, for example. ↩
-
It may turn out we also need something like
projectRoot(e.g.,'packages/core'). In that case, we could either derive it via@nx/devkit'screateProjectGraphAsync, provide the project folder instead and derive its name usingpath.basename, or add another parameter. ↩
| export default createIntConfig( | ||
| 'core', | ||
| { | ||
| projectRoot: new URL('../../', import.meta.url), |
There was a problem hiding this comment.
The "project root" terminology is inconsistent with how Nx uses it. For this example, the project root would be the packages/core directory. So based on the naming, I would expect projectRoot: import.meta.url. The new URL('../..', import.meta.url) value would match what Nx calls the workspace root.
In fact, since the workspace root is the same for each project, I don't see any reason this needs to be a parameter for the factory function. It would be more encapsulated and reliable if the factory function created the absolute path to the workspace root internally.
| test: { | ||
| include: ['src/**/*.{unit,type}.test.{js,mjs,cjs,ts,mts,cts,jsx,tsx}'], | ||
| typecheck: { include: ['**/*.type.test.ts'] }, | ||
| coverage: { exclude: ['perf/**'] }, |
There was a problem hiding this comment.
I would prefer to limit how much variation we have between each project's Vitest configs. Because they were all separate files until now, there are some minor inconsistencies between them. However, in most cases, I don't believe there's a compelling reason for those variations; it just happened to evolve that way.
One such example is the perf folder. Some projects use it, some projects don't. The main thing is we want to stick with this convention for the whole monorepo. Everything in {projectRoot}/perf is non-production code and therefore not relevant for test coverage. Therefore, for both simplicity and consistency, I would simply add coverage: { exclude: ['perf/**'] } to each project, regardless of whether it has a perf folder or not. There's no harm in excluding a folder that doesn't exist.
| include: ['src/**/*.{unit,type}.test.{js,mjs,cjs,ts,mts,cts,jsx,tsx}'], | ||
| typecheck: { include: ['**/*.type.test.ts'] }, |
There was a problem hiding this comment.
In the interest of avoiding overrides per project (see previous comment), I would apply this wider file pattern for each project. Most projects don't happen to use .type.test.ts files, but so what? There's no harm in having them configured. If we decide to add type tests to another project, then it would be most convenient to just start adding files with .type.test.ts extension with no extra configuration effort.
| const CONSOLE_MOCK_PATH = 'testing/test-setup/src/lib/console.mock.ts'; | ||
| const RESET_MOCKS_PATH = 'testing/test-setup/src/lib/reset.mocks.ts'; | ||
|
|
||
| export const setupPresets = { | ||
| unit: { | ||
| base: [ | ||
| CONSOLE_MOCK_PATH, | ||
| RESET_MOCKS_PATH, | ||
| 'testing/test-setup/src/lib/cliui.mock.ts', | ||
| 'testing/test-setup/src/lib/fs.mock.ts', | ||
| 'testing/test-setup/src/lib/extend/ui-logger.matcher.ts', | ||
| ], | ||
| git: ['testing/test-setup/src/lib/git.mock.ts'], | ||
| portalClient: ['testing/test-setup/src/lib/portal-client.mock.ts'], | ||
| matchersCore: [ | ||
| 'testing/test-setup/src/lib/extend/markdown-table.matcher.ts', | ||
| 'testing/test-setup/src/lib/extend/jest-extended.matcher.ts', | ||
| ], | ||
| matcherPath: ['testing/test-setup/src/lib/extend/path.matcher.ts'], | ||
| }, | ||
| int: { | ||
| base: [CONSOLE_MOCK_PATH, RESET_MOCKS_PATH], | ||
| cliui: ['testing/test-setup/src/lib/cliui.mock.ts'], | ||
| fs: ['testing/test-setup/src/lib/fs.mock.ts'], | ||
| git: ['testing/test-setup/src/lib/git.mock.ts'], | ||
| portalClient: ['testing/test-setup/src/lib/portal-client.mock.ts'], | ||
| matcherPath: ['testing/test-setup/src/lib/extend/path.matcher.ts'], | ||
| chromePath: ['testing/test-setup/src/lib/chrome-path.mock.ts'], | ||
| }, | ||
| e2e: { | ||
| base: [RESET_MOCKS_PATH], | ||
| }, | ||
| } as const; |
There was a problem hiding this comment.
I think this is unnecessarily granular. I understand that it may appear like each project needs to pick and choose precisely which mocks and matchers to include. But I believe there should actually be a consistent set for each test type. For example:
- all tests should include
reset.mock.ts - all unit and integration tests should include
console.mock.tsandgit.mock.ts, but no E2E tests should include these mocks - all unit tests should include
fs.mock.ts, but no integration or E2E tests should- I notice there's one exception to this rule -
examples-pluginshave integration tests that rely onfs.mock.ts- that's a f***-up on our side, I've just opened to PR to fix it in test(examples-plugins): do not use memfs in integration tests, intended for unit tests only #1127
- I notice there's one exception to this rule -
- all tests can use any of our custom matchers
In the end, I think the test kind ('unit' | 'int' | 'e2e') should fully determine the list of setupFiles. So the factory functions need not support this override per project.
| - `vitest-config-factory.ts`: builds typed Vitest configs with sensible defaults | ||
| - `vitest-setup-presets.ts`: provides create functions and exportable setup file groups | ||
|
|
||
| The create functions (`createUnitConfig`, `createIntConfig`, `createE2eConfig`) automatically include appropriate setup files for each test type. See the unit tests for detailed documentation of defaults, coverage settings, and setup file presets. |
There was a problem hiding this comment.
I'd rename the exported functions to createUnitTestConfig, createIntTestConfig, and createE2ETestConfig. It would make them more self-descriptive, and it works better in English.
| **Using defaults:** | ||
|
|
||
| ```ts | ||
| export default createUnitConfig('my-package', import.meta.url); | ||
| ``` | ||
|
|
||
| **Extending default setup files:** | ||
|
|
||
| ```ts | ||
| export default createIntConfig('my-package', import.meta.url, { | ||
| setupFiles: [...setupPresets.int.base, ...setupPresets.int.git, './custom-setup.ts'], | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
I would include import statements in these examples. Otherwise, it's not fully clear where createUnitConfig, createIntConfig, and setupPresets are coming from.
| - `vitest-config-factory.ts`: builds typed Vitest configs with sensible defaults | ||
| - `vitest-setup-presets.ts`: provides create functions and exportable setup file groups |
There was a problem hiding this comment.
If I'm only interested in using this project, not maintaining it, then do I need to know about these files? I would think not; they look like implementation details. The project README should first document usage (i.e., your Examples section). Any internal information should be moved to the bottom of the README, as it's least likely to be relevant. That's if it's even needed - IMHO, the file names are pretty self-explanatory, and I can look them up in src if I need to.
|
|
||
| ## Shared config | ||
|
|
||
| [README](./src/lib/config/README.md) how to use vitest config factory. |
There was a problem hiding this comment.
The link is broken, and this doesn't really work as an English sentence.
| [README](./src/lib/config/README.md) how to use vitest config factory. | |
| See [`@code-pushup/test-setup-config` docs](../test-setup-config/README.md) on how to use our Vitest config factory. |
|
|
||
| export function tsconfigPathAliases(): AliasOptions { | ||
| const result = loadConfig('tsconfig.base.json'); | ||
| export function tsconfigPathAliases(projectRootUrl?: URL): AliasOptions { |
There was a problem hiding this comment.
This function is now duplicated in testing/test-setup-config/src/lib/vitest-tsconfig-path-aliases.ts. I'm guessing we don't need it here in tools anymore?
| /.sass-cache | ||
| /connect.lock | ||
| /coverage | ||
| **/.coverage/** |
There was a problem hiding this comment.
Which tool uses .coverage folders? 🤨
3 participants
Centralize and Standardize Vitest Configurations
What Changed
Benefits
The new system provides
createUnitConfig,createIntConfig, andcreateE2eConfigfactory functions that automatically handle common configuration patterns while still allowing customization through override parameters.Relates to first part of #1065