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

Ignore insignificant lines when coalesce #525

Merged
merged 33 commits into from Apr 1, 2020

Conversation

piranna
Copy link
Contributor

@piranna piranna commented Feb 3, 2020

Fixes #514.

Earned 3 extra chars for `MISSING_COL`
Use 80 columns width by default, unlimited if `maxCols` option is set
explicitly to zero
@piranna
Copy link
Contributor Author

piranna commented Mar 24, 2020

Any update on this? Can be reviewed and merged? :-)

@coreyfarrell
Copy link
Member

@piranna sorry for the delay, I've been a bit buried lately but I promise I will review this ASAP.

@piranna
Copy link
Contributor Author

piranna commented Mar 26, 2020

@piranna sorry for the delay, I've been a bit buried lately but I promise I will review this ASAP.

No problem, take your time :-)

Copy link
Member

@coreyfarrell coreyfarrell left a comment

Choose a reason for hiding this comment

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

This looks good to me, I reviewed each change to the tested text reports.

@SimenB @bcoe this changes the output of some text reports so it could break any snapshot testing you have on c8 and jest (maybe less likely for c8). I'll deal with fixing the snapshot tests for nyc but I just wanted to give warning and make sure you don't have strong objections. I'm viewing this as a non-breaking change, I don't think istanbul-reports has an obligation to maintain exact byte-for-byte output of the text report (humans are the target consumer, not computers).

@SimenB
Copy link
Member

SimenB commented Mar 31, 2020

Yeah go ahead! I think it's fair not to regard the visual output as covered by semver - we use a lockfile anyways, so CI won't randomly break for us whenever a release is out 👍

@piranna
Copy link
Contributor Author

piranna commented Mar 31, 2020

This looks good to me, I reviewed each change to the tested text reports.

Thank you for reviewing them :-)

@coreyfarrell
Copy link
Member

This is published to istanbul-report@3.0.2, tagged next for now. I have a PR open for nyc, will probably publish nyc@15.0.1 to next later today.

@piranna very much appreciate your work on this, IMO the updated reports look MUCH better (you can see tap-snapshots changes in the nyc PR).

@piranna
Copy link
Contributor Author

piranna commented Apr 1, 2020

This is published to istanbul-report@3.0.2, tagged next for now. I have a PR open for nyc, will probably publish nyc@15.0.1 to next later today.

Great! :-D

@piranna very much appreciate your work on this, IMO the updated reports look MUCH better (you can see tap-snapshots changes in the nyc PR).

You are welcome :-) I use coverage reports a lot when running Jest tests and was missing to see more missing lines on it and what of them needed more priority (not the best way to do tests, but help to do regression tests). Thank you for accepting it upstream, hope this can be useful for that and to others :-)

@coreyfarrell
Copy link
Member

nyc@15.0.1 is now published to next.

FYI to immediately use this with jest you should be able to explicitly install istanbul-reports@3.0.2 to your own project and this should cause jest to use that version (I'm unsure if you'll need to npm dedupe or similar if jest already installed 3.0.1).

@SimenB
Copy link
Member

SimenB commented Apr 2, 2020

I can bump in jest

@SimenB
Copy link
Member

SimenB commented Apr 2, 2020

jestjs/jest#9758

The changes looks great, awesome job! 👍

@coreyfarrell any idea how long it'll be tagged as next?

@coreyfarrell
Copy link
Member

Unless I get an objection I'm probably going to promote it to latest tomorrow. I just posted a message to our Slack requesting that people test it on projects that do not have 100% coverage.

@coreyfarrell
Copy link
Member

Updates for both istanbul-reports and nyc are now promoted to latest.

@SimenB
Copy link
Member

SimenB commented Apr 3, 2020

Thanks! Merged the Jest PR with updated snapshots now. Not sure when a release will come which forces the update for consumers, but just deleting istanbul-reports from any lockfile you might have should download the latest version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Ignore insignificant lines when coalesce ranges
3 participants