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.

Sign up for GitHub

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

Jump to bottom

adding istanbul ignore for pad2 #229

Merged
merged 1 commit into from
Mar 18, 2020
Merged

adding istanbul ignore for pad2 #229

merged 1 commit into from
Mar 18, 2020

Conversation

ryhinchey
Copy link
Contributor

@ryhinchey ryhinchey commented Feb 27, 2020 •
edited
Loading

Related issue: #228

I'm open to an alternative here. I tried adding sinon@2.0.0 (the oldest version I could install) so I could write a new test with a mocked date; however, sinon wouldn't install correctly in older versions of node #226.

@ryhinchey ryhinchey mentioned this pull request Feb 27, 2020
Closed
@jonchurch jonchurch self-requested a review February 29, 2020 22:57
jonchurch
jonchurch approved these changes Feb 29, 2020
View reviewed changes
Copy link
Member

@jonchurch jonchurch left a comment

Choose a reason for hiding this comment

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

I'm fine with this personally

@dougwilson dougwilson self-assigned this Feb 29, 2020
@dougwilson
Copy link
Contributor

I am also fine with this, and great find @ryhinchey ! I thought there were times in the past I saw the coverage drop randomly but never looked into it that hard... of course the ideal is to test it, but these are one of those things where rewriting it to expose it enough to test would (imo) break encapsulation we'd want to keep private, and some small branch like this we'll just have to skip. This won't be the first place we skip coverage in the various repos, and I think it's a good reason to.

I know that was a lot, but just wanted to provide my reassurance 👍 . I am getting a new machine set up right now and will start chewing through a few things tonight, including getting this landed.

dougwilson pushed a commit to ryhinchey/morgan that referenced this pull request Mar 18, 2020
@ryhinchey @dougwilson
tests: ignore branch coverage that varies
38e747e 
closes expressjs#228
closes expressjs#229
@dougwilson
Copy link
Contributor

Well, the PR just failed due to a falling coverage... so maybe still need to understand better what is happening here

@dougwilson
Copy link
Contributor

Apparently there is a limit to the length of the ignore comment, lol

@dougwilson dougwilson merged commit c54194c into expressjs:master Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dougwilson dougwilson dougwilson approved these changes

@jonchurch jonchurch jonchurch approved these changes

Assignees

@dougwilson dougwilson

Labels
pr tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ryhinchey @dougwilson @jonchurch