bug: Unit test --watchAll argument parsed incorrectly #5640

maxpatiiuk · 2024-04-08T20:24:52Z

Prerequisites

I have read the Contributing Guidelines.
I agree to follow the Code of Conduct.
I have searched for existing issues that already report this problem, without success.

Stencil Version

4.15.0

Current Behavior

Regardless of the argument value, if the argument is present, Stencil sets watch=true:

stencil/src/testing/testing.ts

Lines 235 to 237 in 76c5d54

    
           if (validatedConfig.flags.args.includes('--watchAll')) { 
        
             validatedConfig.watch = true; 
        
           }

This causes this command to be incorrectly interpreted as if watchAll is true:

npx stencil test --spec -- --watchAll=false

Besides confusion, this causes third-party tools to fail or freeze: jest-community/vscode-jest#1124 (comment)

Expected Behavior

If --watchAll is false, it should not be treated as a true

Jest's documentation explicitly mentions this:

Use --no-watchAll (or --watchAll=false) to explicitly disable the watch mode if it was enabled using --watchAll. In most CI environments, this is automatically handled for you.

https://github.com/jestjs/jest/blob/726ca20752e38c18e20aa21740cec7aba7891946/docs/CLI.md?plain=1#L545

For reference, Jest defines it as a boolean argument using the typebox library:
https://github.com/jestjs/jest/blob/726ca20752e38c18e20aa21740cec7aba7891946/packages/jest-schemas/src/raw-types.ts#L346
Which would parse the value as false if it's anything other than true: https://github.com/sinclairzx81/typebox/blob/480ba12942cf908fab6e2ad16b3276b035ddcb54/test/runtime/value/cast/boolean.ts#L5

System Info

System: node 20.11.0
    Platform: darwin (23.3.0)
   CPU Model: Apple M1 Pro (10 cpus)
    Compiler: /Users/mak13180/site/esri/arcgis-web-components/node_modules/@stencil/core/compiler/stencil.js
       Build: 1702922611
     Stencil: 4.15.0 🐏
  TypeScript: 5.2.2
      Rollup: 2.42.3
      Parse5: 7.1.2
      jQuery: 4.0.0-pre
      Terser: 5.26.0

Steps to Reproduce

Run npx stencil test --spec -- --watchAll=false

Code Reproduction URL

https://github.com/maxpatiiuk/vscode-jest-bug-reproduction

Additional Information

No response

The text was updated successfully, but these errors were encountered:

rwaskiewicz · 2024-04-09T12:38:12Z

Thanks! I was able to confirm this is a bug and have labeled it to get it ingested into our backlog.

This fixes two bugs related to parsing of Boolean CLI arguments using the `--argName=value` type syntax. The first is a specific bug related to the `--watchAll` flag, where we previously had an improper special-case for this flag where if it was present in the CLI args in any form than we'd always also set `watch` to true: https://github.com/ionic-team/stencil/blob/76c5d5410068cdb43296911a33b20d47531383a3/src/testing/testing.ts#L235-L237 This isn't what you want if, for instance, you actually typed `--watchAll=false`! See #5640 That in turn also uncovered a bug where we weren't supporting arguments of the form `--booleanArg=false` correctly. We were doing `Boolean('false')` in that case, which does _not_ result in a value of `false` but instead a value of `true`. This is changed to instead use a check that the value equals `'true'` to turn the value into a boolean, and regression tests are added. fixes #5640 STENCIL-1259

This fixes a bug related to parsing of Boolean CLI arguments using the `--argName=value` type syntax. We intend to support using that syntax to pass a boolean CLI flag (like `--watch`) but actually at present the way it is 'parsed' will result in such flags always being set to true! Not good! This is because we were setting the value of the boolean CLI flag to `Boolean('false')` if you pass `--argName=false`, which does _not_ result in a value of `false` but instead a value of `true`. This is changed to instead use a check that the value equals `'true'` to turn the value into a boolean, and regression tests are added. This was noticed when investigating #5640, but that bug actually has to do with how we convert CLI arguments into a Jest configuration.

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). 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

alicewriteswrongs · 2024-04-15T15:30:29Z

Hey @maxpatiiuk I just put up a PR (#5672) which I believe fixes this issue. I published a dev build that includes the change if you want to try it out!

You can install it in a stencil project like so:

npm i @stencil/core@4.16.0-dev.1713194811.e1f6157

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

…5672) 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

ionitron-bot bot added the triage label Apr 8, 2024

rwaskiewicz assigned rwaskiewicz and unassigned rwaskiewicz Apr 9, 2024

rwaskiewicz added the Bug: Validated This PR or Issue is verified to be a bug within Stencil label Apr 9, 2024

ionitron-bot bot removed the triage label Apr 9, 2024

alicewriteswrongs mentioned this issue Apr 10, 2024

fix(cli): fix a bug in CLI argument parsing #5646

Merged

2 tasks

tanner-reits mentioned this issue Apr 15, 2024

Release v4.16.0 #5671

Merged

alicewriteswrongs mentioned this issue Apr 15, 2024

fix(testing): perform string -> boolean type coercion for Jest config #5672

Merged

2 tasks

jcfranco mentioned this issue Apr 16, 2024

build(deps): update dependency @stencil/core to v4.16.0 Esri/calcite-design-system#9119

Merged

1 task

alicewriteswrongs closed this as completed in #5672 Apr 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug: Unit test --watchAll argument parsed incorrectly #5640

bug: Unit test --watchAll argument parsed incorrectly #5640

maxpatiiuk commented Apr 8, 2024

rwaskiewicz commented Apr 9, 2024

alicewriteswrongs commented Apr 15, 2024

bug: Unit test --watchAll argument parsed incorrectly #5640

bug: Unit test --watchAll argument parsed incorrectly #5640

Comments

maxpatiiuk commented Apr 8, 2024

Prerequisites

Stencil Version

Current Behavior

Expected Behavior

System Info

Steps to Reproduce

Code Reproduction URL

Additional Information

rwaskiewicz commented Apr 9, 2024

alicewriteswrongs commented Apr 15, 2024