bug: --reporters flag broken #3712
Comments
|
Hey @johnjenkins 👋 Thanks for the detailed reproduction report! I modified your test running command ever so slightly to verify this without Jest's caching getting in the way: - npm run test -- --coverage --reporters="default" --reporters="jest-junit"
+ npm run test -- --coverage --reporters="default" --reporters="jest-junit" --no-cacheand I believe that this was introduced somewhere in v2.17.X: v2.17.0 (working)$ npm i @stencil/[email protected] changed 1 package, and audited 377 packages in 775ms From that build above, I can run v2.17.2 (broken)$ npm i @stencil/[email protected] And do junit output is generated for v2.17.2 I'm going to get this ingested for us to take a closer look at. |
rwaskiewicz
added
the
Bug: Validated
|
@johnjenkins after looking into this a bit I decided that in order to address this we needed to refactor Stencil's CLI parser in order to properly support this. A PR to do so is up here: #3765 It's not yet in a prereleased, I'll ping this issue when it merges and is ready for testing! |
This commit refactors the argument parser we use for our CLI module.
Doing so fixes an issue with certain flags supported by Jest which can
be passed multiple times. For instance, in the following example:
```
jest --coverage --reporters="default" --reporters="jest-junit"
```
all of the values for the `--reporters` flag ("default" and
"jest-junit") should be collected into an array of values, instead of
simply recording whichever value is farther to the right (Stencil's
behavior before this commit).
To support passing such arguments to the `stencil test` subcommand this
commit adds a new recursive-descent parser in `src/cli/parse-flags.ts`
to replace the somewhat ad-hoc approach that was there previously. It
parses the following grammar:
```
CLIArguments → ""
| CLITerm ( " " CLITerm )* ;
CLITerm → EqualsArg
| AliasEqualsArg
| AliasArg
| NegativeDashArg
| NegativeArg
| SimpleArg ;
EqualsArg → "--" ArgName "=" CLIValue ;
AliasEqualsArg → "-" AliasName "=" CLIValue ;
AliasArg → "-" AliasName ( " " CLIValue )? ;
NegativeDashArg → "--no-" ArgName ;
NegativeArg → "--no" ArgName ;
SimpleArg → "--" ArgName ( " " CLIValue )? ;
ArgName → /^[a-zA-Z-]+$/ ;
AliasName → /^[a-z]{1}$/ ;
CLIValue → '"' /^[a-zA-Z0-9]+$/ '"'
| /^[a-zA-Z0-9]+$/ ;
```
The regexes are a little fuzzy, but this is sort of an informal
presentation, and additionally there are other constraints implemented
in the code which handles all of these different terms.
Refactoring this to use a proper parser (albeit a pretty simple one)
allows our implementation to much more clearly conform to this defined
grammar, and should hopefully both help us avoid other bugs in the
future and be easier to maintain.
See #3712 for more details on the issues with the `--reporters` flag in
particular.
This commit refactors the argument parser we use for our CLI module.
Doing so fixes an issue with certain flags supported by Jest which can
be passed multiple times. For instance, in the following example:
```
jest --coverage --reporters="default" --reporters="jest-junit"
```
all of the values for the `--reporters` flag ("default" and
"jest-junit") should be collected into an array of values, instead of
simply recording whichever value is farther to the right (Stencil's
behavior before this commit).
To support passing such arguments to the `stencil test` subcommand this
commit adds a new recursive-descent parser in `src/cli/parse-flags.ts`
to replace the somewhat ad-hoc approach that was there previously. It
parses the following grammar:
```
CLIArguments → ""
| CLITerm ( " " CLITerm )* ;
CLITerm → EqualsArg
| AliasEqualsArg
| AliasArg
| NegativeDashArg
| NegativeArg
| SimpleArg ;
EqualsArg → "--" ArgName "=" CLIValue ;
AliasEqualsArg → "-" AliasName "=" CLIValue ;
AliasArg → "-" AliasName ( " " CLIValue )? ;
NegativeDashArg → "--no-" ArgName ;
NegativeArg → "--no" ArgName ;
SimpleArg → "--" ArgName ( " " CLIValue )? ;
ArgName → /^[a-zA-Z-]+$/ ;
AliasName → /^[a-z]{1}$/ ;
CLIValue → '"' /^[a-zA-Z0-9]+$/ '"'
| /^[a-zA-Z0-9]+$/ ;
```
The regexes are a little fuzzy, but this is sort of an informal
presentation, and additionally there are other constraints implemented
in the code which handles all of these different terms.
Refactoring this to use a proper parser (albeit a pretty simple one)
allows our implementation to much more clearly conform to this defined
grammar, and should hopefully both help us avoid other bugs in the
future and be easier to maintain.
See #3712 for more details on the issues with the `--reporters` flag in
particular.
|
Hey @johnjenkins 👋 @alicewriteswrongs' fix for this (#3765) has been included Stencil v2.19.3, which was just released this morning. I verified the fix one last time using the released version of Stencil and your reproduction case: Original Reproduction with Stencil v2.18.1: Using Stencil v2.19.3 As a result, I'm going to close this issue. Please feel free to open a new issue if this bug resurfaces. Thanks again for the detailed bug report! |
|
Hello @rwaskiewicz, I still have problem with the CLI. I used to do My I also used to add the |
|
Hey @duhem-s 👋 It sounds like there's one, potentially two different issues occurring there. For each issue, can you please open a new issue in the repo with a minimal reproduction case for the team to take a look at? Thanks! |
|
@rwaskiewicz Here is the issue : #3825 Should I create a new one for the |
|
@duhem-s can you please create a new issue? |
|
@rwaskiewicz Just did it : #3828 |
Prerequisites
Stencil Version
~2.15
Current Behavior
When attempting to use the
--reporters(https://jestjs.io/docs/cli#--reporters) flag from the cli, it throws an error[ ERROR ] runJest: TypeError: reporters.find is not a functionExpected Behavior
no error :)
Steps to Reproduce
npm init stencilcd into the project:
npm iadd another reporter:
npm i jest-junittry to run your tests:
npm run test -- --coverage --reporters="default" --reporters="jest-junit"Code Reproduction URL
https://github.com/johnjenkins/stencil-reporters-flag-bug
Additional Information
It's the same issue with jest-* at v26 or v27.
If you downgrade stencil (to ~2.13 / jest-* at v26), the above steps work
The text was updated successfully, but these errors were encountered: