-
Notifications
You must be signed in to change notification settings - Fork 789
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
fix(testing): perform string -> boolean type coercion for Jest config #5672
Conversation
PR built and packed!Download the tarball here: https://github.com/ionic-team/stencil/actions/runs/8785317808/artifacts/1435751495 If your browser saves files to
|
|
Path | Error Count |
---|---|
src/dev-server/index.ts | 37 |
src/dev-server/server-process.ts | 32 |
src/compiler/prerender/prerender-main.ts | 22 |
src/runtime/client-hydrate.ts | 20 |
src/testing/puppeteer/puppeteer-element.ts | 20 |
src/screenshot/connector-base.ts | 19 |
src/runtime/vdom/vdom-render.ts | 17 |
src/dev-server/request-handler.ts | 15 |
src/compiler/prerender/prerender-optimize.ts | 14 |
src/compiler/sys/stencil-sys.ts | 14 |
src/sys/node/node-sys.ts | 14 |
src/compiler/prerender/prerender-queue.ts | 13 |
src/compiler/sys/in-memory-fs.ts | 13 |
src/runtime/connected-callback.ts | 13 |
src/runtime/set-value.ts | 13 |
src/compiler/output-targets/output-www.ts | 12 |
src/compiler/transformers/test/parse-vdom.spec.ts | 12 |
src/compiler/transformers/transform-utils.ts | 12 |
src/compiler/transpile/transpile-module.ts | 12 |
src/mock-doc/test/attribute.spec.ts | 12 |
Our most common errors
Typescript Error Code | Count |
---|---|
TS2322 | 361 |
TS2345 | 344 |
TS18048 | 204 |
TS18047 | 82 |
TS2722 | 37 |
TS2532 | 24 |
TS2531 | 21 |
TS2454 | 14 |
TS2790 | 11 |
TS2352 | 9 |
TS2769 | 8 |
TS2538 | 8 |
TS2416 | 7 |
TS2493 | 3 |
TS18046 | 2 |
TS2684 | 1 |
TS2430 | 1 |
Unused exports report
There are 14 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!
Unused exports
File | Line | Identifier |
---|---|---|
src/runtime/bootstrap-lazy.ts | 21 | setNonce |
src/screenshot/screenshot-fs.ts | 18 | readScreenshotData |
src/testing/testing-utils.ts | 198 | withSilentWarn |
src/utils/index.ts | 145 | CUSTOM |
src/utils/index.ts | 269 | normalize |
src/utils/index.ts | 7 | escapeRegExpSpecialCharacters |
src/compiler/app-core/app-data.ts | 25 | BUILD |
src/compiler/app-core/app-data.ts | 115 | Env |
src/compiler/app-core/app-data.ts | 117 | NAMESPACE |
src/compiler/fs-watch/fs-watch-rebuild.ts | 123 | updateCacheFromRebuild |
src/compiler/types/validate-primary-package-output-target.ts | 61 | satisfies |
src/compiler/types/validate-primary-package-output-target.ts | 61 | Record |
src/testing/puppeteer/puppeteer-declarations.ts | 485 | WaitForEventOptions |
src/compiler/sys/fetch/write-fetch-success.ts | 7 | writeFetchSuccessSync |
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.
The functionality here LGTM - one question/comment regarding changing the build infra here and why we need to do it as a part of this PR
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.
One non-blocking ask - #5672 (comment)
e1f6157
to
ee8a577
Compare
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.
One quick question
This adds some manual string -> boolean type casting to the code that we use to convert CLI flags to a Jest config (after parsing them with yargs). This fixes an issue documented in #5640 where boolean CLI flags like `--watchAll` were being passed to Jest as strings, instead of as booleans, so that it was not possible to _disable_ a flag by doing `--flagName=false` (the value of the flag would be set to the string `"false"` which is a truthy value). There was also a somewhat related issue with not properly parsing boolean flags in our CLI parser which was recently fixed in #5646. Note that to get this working it was necessary to make some module resolution-related changes in a few places since there's now a dependency between the `testing/` and `cli/` modules (`testing/` now imports and using `BOOLEAN_CLI_FLAGS`). This involved a `paths` alias in the TypeScript config, esbuild- and rollup-specific module mapping configuration / aliases, and a change in the Jest config to allow `@stencil/core/cli` to be resolved in all these contexts. fixes #5640 STENCIL-1259
ee8a577
to
ff5c771
Compare
Released in Stencil ♨️ v4.17.0 |
This adds some manual string -> boolean type coercion to the code that we use to convert CLI flags to a Jest config (after parsing them with yargs).
This fixes an issue documented in #5640 where boolean CLI flags like
--watchAll
were being passed to Jest as strings, instead of as booleans, so that it was not possible to disable a flag by doing--flagName=false
(the value of the flag would be set to the string"false"
which is a truthy value).We previously did this only for a few special-cased things, like so:
stencil/src/testing/jest/jest-27-and-under/jest-config.ts
Lines 84 to 86 in 2082368
There was also a somewhat related issue with not properly parsing boolean flags in our CLI parser which was recently fixed in #5646.
fixes #5640
STENCIL-1259
What is the current behavior?
We don't properly coerce strings like
'true'
,'false'
to booleans when constructing Jest CLI arguments. This results in a situation where if you pass something like--watchAll=false
you'll actually end up running tests in watch mode, which isn't what you want!This fixes that behavior by explicitly coercing strings to booleans when they're in our list of defined boolean CLI flags.
Does this introduce a breaking change?
Testing
First I think it's good to confirm you can reproduce the error.
Basically, start a test project w/
@stencil/core@latest
installed and donpx stencil test --spec -- --watchAll=false
you should observe that this starts things in watch mode, contrary to what you might expect.
then build, pack, and install this branch in the same project and run the same command. It should now run the tests without starting watch mode.