search

expressjs / morgan

HTTP request logger middleware for node.js
MIT License
7.95k stars 536 forks source link

adding istanbul ignore for pad2 #229

Closed ryhinchey closed 4 years ago

ryhinchey commented 4 years ago

Related issue: https://github.com/expressjs/morgan/issues/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 https://github.com/expressjs/morgan/pull/226.

dougwilson commented 4 years ago

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 commented 4 years ago

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

dougwilson commented 4 years ago

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