chore(tests): pro 168 refactor tests #842
Conversation
alvarosabu
had a problem deploying
to
production
GitHub Actions
Failure
tests/index.e2e.ts
Outdated
| @@ -0,0 +1,147 @@ | |||
| import StoryblokClient from '../src/' | |||
There was a problem hiding this comment.
@alvarosabu while in the unit tests is perfectly fine, as this is an integration test (api tests to be more specific) it shouldn't use the src, but rather the compiled version of the client
There was a problem hiding this comment.
Good point. Thanks
vitest.config.ts
Outdated
| export default defineConfig({ | ||
| test: { | ||
| include: ['./tests/**/*.test.ts'], | ||
| exclude: ['./old-tests'], |
There was a problem hiding this comment.
Same here.
Also, more as an opinion, how would you see to separate the tests in tests/unit and test/api (or test/e2e) folders? I feel it can be more clearly organized like that
There was a problem hiding this comment.
Hey @alexjoverm, good point, what about having the unit tests in the same place as the code and API tests on the e2e folder? Seems to be a common practice between test advocates
--- index.ts
--- index.test.ts
There was a problem hiding this comment.
I wouldn't be against - to me I also prefer that practice as it's more modular, and at the end, a unit test it's tightly couple to the "component" that is testing.
Only thing is that we'd be introducing an inconsistency with the other packages as they have it in a test folder - but we can pioneer the new approach from js-client, and later in time apply it to other pkgs when we update tests there. What do you think?
There was a problem hiding this comment.
Agree. Mmm, since we already introduced the pnpm workspaces setup, I think it's worth start propagating these new practices down.
alvarosabu
had a problem deploying
to
production
GitHub Actions
Failure
alvarosabu
had a problem deploying
to
production
GitHub Actions
Failure
alvarosabu
had a problem deploying
to
production
GitHub Actions
Failure
alvarosabu
had a problem deploying
to
production
GitHub Actions
Failure
alvarosabu
had a problem deploying
to
production
GitHub Actions
Failure
|
🎉 This PR is included in version 6.8.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
|
||
| strategy: | ||
| matrix: | ||
| node-version: [20] |
There was a problem hiding this comment.
The matrix strategy for such a workflow is redundant - you wouldn't (and can't) publish a package with multiple Node versions.
There was a problem hiding this comment.
Hi @ryami333 how would you suggest to setup the CI instead? Could you create a PR with your approach to discuss it?
We normally keep node versions to the latest LTS, do you see it beneficial to aim for multiple node versions?
Thanks!
There was a problem hiding this comment.
The purpose of matrix strategies is "Running variations of jobs in a workflow". Configuring this job with a matrix strategy is redundant for two reasons:
- You've only put one element (20) in the matrix, so it's effectively the same as running it without a matrix strategy at all. You could have just written the Node version directly in the
actions/setup-nodestep, as it was before this PR. - You wouldn't want to add any other versions to this particular workflow's matrix, because you wouldn't be able be publish the package two-or-more times. For "testing" / "linting" workflows it makes sense to run the workflow with multiple Node versions (eg.
[18, 20, …etc]) - once for each Node version that the library officially supports. But again - you can't publish a library multiple times.
Pull request type
Jira Link: INT-
How to test this PR
Unit tests
E2E