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

fix(testing): issue passing coverageRepoters commandline #2645

Closed

Conversation

kristofdegrave
Copy link
Contributor

In Jest coverageReporters are an array of strings (https://jestjs.io/docs/en/configuration#coveragereporters-arraystring). In here it is an optional string.

Current Behavior (This is the behavior we have today, before the PR is merged)

When passing a string value for the coverageReporters to the command line, it results into the following error:

Failed to write coverage reports:
ERROR: TypeError: coverageReporters.forEach is not a function
STACK: TypeError: coverageReporters.forEach is not a function

Expected Behavior (This is the new behavior we can expect after the PR is merged)

Use the provided coverageReporter

Issue

In Jest coverageReporters are an array of strings (https://jestjs.io/docs/en/configuration#coveragereporters-arraystring). In here it is an optional string.
When passing a string value for the coverageReporters to the command line, it results into the following error:

Failed to write coverage reports:
ERROR: TypeError: coverageReporters.forEach is not a function
STACK: TypeError: coverageReporters.forEach is not a function
@vsavkin
Copy link
Member

vsavkin commented Mar 23, 2020

Thank you so much for your PR!

I'm not sure if the fix is right. options.coverageReporters is plural, so you should be able to pass more than one, right? I'd update the unit tests to make sure that use case is covered.

@kristofdegrave
Copy link
Contributor Author

You are correct, but didn't want to change the current contract, because I'm not sure if it would break with other testing frameworks. I think the angular cli only allows one reporter. If it is Ok for you I can change it.

@dgsmith2
Copy link

This problem/fix here seems very much like #1854.

I'm working on some code to restore --coverage (#2569) because it falls into the same use case as #1852.

Is there anything I can do to help this move along?

@vsavkin
Copy link
Member

vsavkin commented May 22, 2020

Sorry for the late reply, folks. I'm a bit confused about the current state. You are saying that currently the coverageReporters option cannot be used, right? It only accepts a string, and Jest expect an array. Is this the case in Nx 9.3?

@kristofdegrave
Copy link
Contributor Author

kristofdegrave commented May 25, 2020

It still seems to be a single string

"coverageReporters": {
"description": "A list of reporter names that Jest uses when writing coverage reports. Any istanbul reporter",
"type": "string"
},

coverageReporters?: string;

@vsavkin vsavkin force-pushed the master branch 3 times, most recently from 738e12c to d1679ce Compare May 29, 2020 18:43
@vsavkin
Copy link
Member

vsavkin commented Jun 18, 2020

I'm really sorry folks for the late reply. I think we should change the schema to accept an array of string instead of wrapping the provided value into an array.

"coverageReporters": {
"description": "A list of reporter names that Jest uses when writing coverage reports. Any istanbul reporter",
"type": "array",
"items": {
"type": "string"
}
}

With this change, we will be able to do:

nx test myproj --coverageReporters=json,text

@vsavkin vsavkin self-assigned this Jun 18, 2020
@kristofdegrave
Copy link
Contributor Author

@vsavkin that should be the best solution :). But I wasn't sure that it would conflict with the schema of the angular-cli. That is why I solved it this way.

@vsavkin
Copy link
Member

vsavkin commented Jul 10, 2020

Changing the schema of the jest builder won't affect or by affected by the Angular CLI. We aren't wrapping an existing builder, so we can come up with any schema we want.

@vsavkin
Copy link
Member

vsavkin commented Jul 22, 2020

@kristofdegrave, please let me know if you need any help with this. I know I haven't been the quickest communicator here, so if you don't have time to finish it up, it's totally OK. I can do it in this case, but I'll keep you as a commit author.

@vsavkin
Copy link
Member

vsavkin commented Aug 31, 2020

Sorry folks, but I'm going to close this PR. I'm trying to keep the PR list short to reflect the things that are currently being worked on

@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants