Add @knapsack-pro/vitest by BenLorantfy · Pull Request #104 · KnapsackPro/knapsack-pro-js

BenLorantfy · 2024-12-07T23:05:05Z

Adds @knapsack/vitest package

Closes #102

I tagged with the correct @knapsack-pro/PACKAGE label
I added my changes to the Unreleased section in the CHANGELOG(s)

ArturT · 2024-12-09T14:02:32Z

@BenLorantfy thank you for the PR. We have prioritized it and will review it soon.

ArturT

I've added some comments.

Could you rename the PR title to: Add @knapsack-pro/vitest. There is a missing suffix: -pro.

You can also click on a request to run the workflow so that we can run the CircleCI build for this PR. I think this link should be somewhere in the Conversation tab in the right menu. docs I'd approve it then.

Thank you. Please let me know if I can help with anything.

ArturT · 2024-12-10T22:10:35Z

+
+2. Update your CI to use a matrix build. See the [example workflow](.github/workflows/vitest-example-test-suite.yaml) for reference.
+3. Replace `vitest` with `@knapsack-pro/vitest` in your CI config
+4. Set the `KNAPSACK_PRO_TEST_SUITE_TOKEN` environment variable to your Knapsack Pro test suite token, and optionally set other environemnt variables as needed.


Please could you use _VITEST suffix (KNAPSACK_PRO_TEST_SUITE_TOKEN_VITEST) so it's consistent with other test runners?

Thanks for the review @ArturT 🙏

Do you mind if I push back a little bit on this comment? I agree it's consistent, but I find this approach a bit odd:

knapsack-pro-js/packages/jest/src/env-config.ts

Lines 3 to 6 in 1deb629

if (process.env.KNAPSACK_PRO_TEST_SUITE_TOKEN_JEST) {

process.env.KNAPSACK_PRO_TEST_SUITE_TOKEN =

process.env.KNAPSACK_PRO_TEST_SUITE_TOKEN_JEST;

}

Generally I don't think it's recommended to set env variables like this during runtime. I asked v0 about this and it agreed: https://v0.dev/chat/fOuT0uzgMOL

Besides the general unexpectedness of this pattern, I also think it's a bit odd that only this env variable has a _VITEST suffix, and no other knapsack env variable has one.

So I would suggest this instead:

if (process.env.KNAPSACK_PRO_TEST_SUITE_TOKEN_VITEST) { throw new Error("Please use KNAPSACK_PRO_TEST_SUITE_TOKEN instead of KNAPSACK_PRO_TEST_SUITE_TOKEN_VITEST"); }

That way if someone tries to use KNAPSACK_PRO_TEST_SUITE_TOKEN_VITEST, it will tell them to use KNAPSACK_PRO_TEST_SUITE_TOKEN. I hope that helps mitigate the lack of consistency, by helping consumers find the right env variable

What do you think?

Do you mind if I push back a little bit on this comment? I agree it's consistent, but I find this approach a bit odd:
...

I agree with you. It's odd, and we don't like overriding the env var.

I'd like to break this into 2 separate steps.

For this PR, let's keep the old approach for consistency. Why it's important?
We would like to optimize for the end user's convenience. Some users run 2 different test runners on CI. They might run RSpec and Vitest. They use:

KNAPSACK_PRO_TEST_SUITE_TOKEN_RSPEC

KNAPSACK_PRO_TEST_SUITE_TOKEN_VITEST

The user can just add these to CI settings and that's it. For example, this is possible for CircleCI.
Otherwise, the user would add 2 env vars with unique names to the CI settings, then redefine them in the CI yml file to set KNAPSACK_PRO_TEST_SUITE_TOKEN. This requires extra work from the user. We would like the user experience to be smooth.

I think the long-term approach should be to change how the @knapsack-pro/core package works so that we don't need to redefine env var like this. But I'd like to keep it outside of the scope of this PR. We keep internal notes for that. Is that fine with you?

Sure, sounds good 👍

ArturT · 2024-12-11T13:23:15Z

We could also update the main README file to:

diff --git a/README.md b/README.md
index 448744b..1db3de5 100644
--- a/README.md
+++ b/README.md
@@ -163,8 +163,10 @@ Read the READMEs inside the nested `packages/`:

 - [core](https://github.com/KnapsackPro/knapsack-pro-js/tree/main/packages/core)
 - [jest](https://github.com/KnapsackPro/knapsack-pro-js/tree/main/packages/jest)
+- [vitest](https://github.com/KnapsackPro/knapsack-pro-js/tree/main/packages/vitest)
 - [cypress](https://github.com/KnapsackPro/knapsack-pro-js/tree/main/packages/cypress)
 - [jest-example-test-suite](https://github.com/KnapsackPro/knapsack-pro-js/tree/main/packages/jest-example-test-suite)
+- [vitest-example-test-suite](https://github.com/KnapsackPro/knapsack-pro-js/tree/main/packages/vitest-example-test-suite)
 - [cypress-example-test-suite](https://github.com/KnapsackPro/knapsack-pro-js/tree/main/packages/cypress-example-test-suite)
 - [create-react-app-example](https://github.com/KnapsackPro/knapsack-pro-js/tree/main/packages/create-react-app-example)

ArturT · 2024-12-11T14:07:12Z

I've been trying to run vitest tests locally. I've added a missing configuration for it on a separate branch:

Feel free to cherry pick the following commit into this PR:

commit

I run tests locally this way:

npm run test:vitest

Everything seems to be working fine but after tests are completed then the process does not exit. It hangs. If I press enter then I'm getting this error:

 RERUN  rerun all tests


⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Unhandled Errors ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

Vitest caught 1 unhandled error during the test run.
This might cause false positive tests. Resolve unhandled errors to make sure your tests are not affected.

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Unhandled Error ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
TypeError: Cannot read properties of undefined (reading 'config')
 ❯ WorkspaceProject.getSerializableConfig ../../node_modules/vitest/dist/chunks/cli-api.C2yC_ESk.js:10130:19
 ❯ getConfig ../../node_modules/vitest/dist/chunks/resolveConfig.RxKrDli4.js:6796:33
 ❯ ../../node_modules/vitest/dist/chunks/resolveConfig.RxKrDli4.js:6817:19
 ❯ Object.runTests ../../node_modules/vitest/dist/chunks/resolveConfig.RxKrDli4.js:6814:21
 ❯ executeTests ../../node_modules/vitest/dist/chunks/resolveConfig.RxKrDli4.js:7680:5
 ❯ ../../node_modules/vitest/dist/chunks/cli-api.C2yC_ESk.js:10798:9
 ❯ Vitest.runFiles ../../node_modules/vitest/dist/chunks/cli-api.C2yC_ESk.js:10820:12
 ❯ Vitest.rerunFiles ../../node_modules/vitest/dist/chunks/cli-api.C2yC_ESk.js:10866:5

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

 Test Files  4 passed (4)
      Tests  4 passed (4)
     Errors  1 error
   Start at  14:57:56
   Duration  20.09s

 % Coverage report from v8
----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
----------|---------|----------|---------|---------|-------------------
All files |       0 |        0 |       0 |       0 |
 add.ts   |       0 |        0 |       0 |       0 | 1-3
----------|---------|----------|---------|---------|-------------------
 FAIL  Tests failed. Watching for file changes...
       press h to show help, press q to quit
Cancelling test run. Press CTRL+c again to exit forcefully.

npm error Lifecycle script `test:ks` failed with error:
npm error code 130
npm error path /Users/artur/Documents/github/knapsack-pro/knapsack-for-js/knapsack-pro-js/packages/vitest-example-test-suite
npm error workspace @knapsack-pro/vitest-example-test-suite@1.0.0
npm error location /Users/artur/Documents/github/knapsack-pro/knapsack-for-js/knapsack-pro-js/packages/vitest-example-test-suite
npm error command failed
npm error command sh -c bin/knapsack_pro_vitest

I think vitest starts in watch mode by default. The message Watching for file changes... suggest it.
I think we need to configure Vitest to not start in watch mode when running via Knapsack Pro.
@BenLorantfy Have you encountered this issue before?

BenLorantfy · 2024-12-12T03:21:04Z

Hi @ArturT , thanks for the review

I think I've addressed or responded to all your comments.

I think we need to configure Vitest to not start in watch mode when running via Knapsack Pro.

Yes vitest runs in watch mode unless process.env.CI is set. I've pushed a change to disable watch mode regardless of the CI env var:

knapsack-pro-js/packages/vitest/src/knapsack-vitest.ts

Line 46 in 95ce5fc

watch: false,

You can also click on a request to run the workflow so that we can run the CircleCI build for this PR

I'm having trouble finding this option. Is it in github somewhere or it's in circleci?

ArturT · 2024-12-12T15:46:17Z

  },
  "dependencies": {
-    "@knapsack-pro/core": "^7.1.0",
+    "@knapsack-pro/core": "^8.0.0",


Can we keep it as it was? I'd like to avoid coupling your PR with updating other clients. The same case is for the Jest client, which is down below.

So I was running into an issue where working locally, @knapsack-pro/vitest wasn't using the version in the monorepo. npm was installing version 7.1.0, instead of linking to the monorepo version, since the version in the monorepo was 8.0.0

But sure I'll revert this

ArturT

@BenLorantfy Thank you for the improvements. I appreciate it.

I've managed to enable CircleCI for this PR. We have some linter errors:
https://app.circleci.com/pipelines/github/KnapsackPro/knapsack-pro-js/375/workflows/9d028f9b-5244-47f8-b1eb-dfb9da9db097/jobs/343

Would you be able to help resolve these errors? If you need any help with this or with any of my comments just let me know.

ArturT · 2024-12-12T18:03:19Z

Let's also add packages/vitest/CHANGELOG.md. I've noticed we forgot about it.

# Changelog

## Unreleased

## 0.1.0

- Create Vitest integration

BenLorantfy · 2024-12-13T00:51:49Z

thanks @ArturT , I think I've addressed all your comments. Please lmk if you have any other feedback

ArturT

Awesome work! I'm happy that we will be able to help Vitest users. Thank you @BenLorantfy for your contribution.

I'll take care of releasing the package.

We will update our docs/dashboard in a separate step.

Add @knapsack-pro/vitest

52eeab1

BenLorantfy force-pushed the add-knapsack-vitest branch from c5cdff7 to 52eeab1 Compare December 7, 2024 23:18

BenLorantfy marked this pull request as ready for review December 7, 2024 23:19

BenLorantfy commented Dec 7, 2024

View reviewed changes

Comment thread .github/workflows/vitest-example-test-suite.yaml

ArturT added the @knapsack-pro/vitest label Dec 10, 2024

ArturT requested changes Dec 10, 2024

View reviewed changes

ArturT reviewed Dec 11, 2024

View reviewed changes

Comment thread packages/vitest/LICENSE Outdated

ArturT reviewed Dec 11, 2024

View reviewed changes

Comment thread packages/vitest/LICENSE

BenLorantfy changed the title ~~Add @knapsack/vitest~~ Add @knapsack-pro/vitest Dec 12, 2024

BenLorantfy added 2 commits December 11, 2024 21:14

Implement CR comments

32719de

Add missing configuration items and disable watch

95ce5fc

BenLorantfy added 2 commits December 11, 2024 23:26

Also disable watch when resolving config

a03244c

Misc tweaks

9870e2a

BenLorantfy requested a review from ArturT December 12, 2024 04:31