Skip to content
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

Migrate from jest to node runner #413

Merged
merged 10 commits into from
Feb 25, 2025
Merged

Migrate from jest to node runner #413

merged 10 commits into from
Feb 25, 2025

Conversation

daogrady
Copy link
Contributor

@daogrady daogrady commented Feb 24, 2025

Replace Jest runner with native Node runner. This allows us to remove all Jest-related dependencies.

Most test suits are a 1:1 migration. But runtime.test.ts is a bit tricky. That suite's purpose is to simulate actually writing a service in TypeScript. As we are getting rid of Jest, and therefore ts-jest as well, that is no longer easily testable with just node --test. The test therefore consists of a transpilation step, and then testing the generated .js file.
So successfully running tsc + node --test on runtime.test.ts should achieve the same as just running the file through ts-jest.

@daogrady daogrady added the skip changelog Do not run "changelog included" check for this PR label Feb 24, 2025
@daogrady daogrady requested a review from chgeo February 24, 2025 17:59
"test:integration": "node --test ./test/postinstall.integrationtest.js",
"test:runtime": "npm run test:runtime:post-mortem; rm -rf test/typescript/_out",
"test:runtime:tsc": "npx tsc test/typescript/runtime.test.ts --outDir test/typescript/_out --target ES2020 --module commonjs --esModuleInterop",
"test:runtime:post-mortem": "npm run test:runtime:tsc && node --test './test/typescript/_out/runtime.test.js'",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

script to run the transpiling test, but not clean up after it. Should be run manually and with intent. Enables the user to inspect the generated files, while avoiding clutter when run regularly, where a cleanup step is included (test:runtime).

expect(!ua.is('authenticated-user'))
assert.strictEqual(ua.id, 'foo')
assert(ua.is('any'))
assert(ua.is('authenticated-user'))
Copy link
Contributor Author

@daogrady daogrady Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this actually seems to have changed semantics. Judging by the test, a new User was not supposed to be authenticated before. But an inspection in the debugging tools revealed they actually were (now?). I think I faintly recall something from recent release notes, but I am not 100% sure and did not find anything. @chgeo do you know if this has changed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. Runtime folks might know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mariayord are you aware if new User().is('authenticated-user') yielding true is expected behaviour and if this has changed recently?

➜  cds r
Welcome to cds repl v 8.7.2 
> new cds.User().is('authenticated-user')
true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @daogrady , I have synced with @sjvans. This is the expected behavior.
I am not sure if this has been changed recently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for checking, @mariayord ! The test was inversed before, so I guess it has to have changed at some point, but it is good that it is now in line with the actual behaviour. 🙂

Copy link
Member

@chgeo chgeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check what the cap/dev tests do after this change when they run all tests here w/ Jest.

@daogrady
Copy link
Contributor Author

daogrady commented Feb 25, 2025

Please check what the cap/dev tests do after this change when they run all tests here w/ Jest.

Thanks for pointing this out. Running jest directly in cds-types will cause errors. I can not speak with certainty wrt the dev setup, as I was not able to fully run the tests locally.
To be safe, I have prepared cap/dev/pull/1976 to swiftly deactivate the cds-types tests in our internal pipeline if it should cause problems.

@chgeo
Copy link
Member

chgeo commented Feb 25, 2025

To be safe, I have prepared cap/dev/pull/1976 to swiftly deactivate the cds-types tests in our internal pipeline if it should cause problems.

Yes. See my comments in cap/dev/pull/1976

@daogrady daogrady merged commit b1d5f10 into main Feb 25, 2025
19 checks passed
@daogrady daogrady deleted the chore/jest-to-node-runner branch February 25, 2025 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changelog Do not run "changelog included" check for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants