Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3289 +/- ##
==========================================
+ Coverage 97.40% 98.19% +0.78%
==========================================
Files 22 22
Lines 10771 10658 -113
Branches 1866 1848 -18
==========================================
- Hits 10492 10466 -26
+ Misses 258 175 -83
+ Partials 21 17 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jessealama
force-pushed
the
temporal-coverage-gaps
branch
from
bfb52ce to
560ef50
Compare
ptomato
left a comment
ptomato
left a comment
There was a problem hiding this comment.
Thanks!
Good in principle, but needs a few adjustments. There are a few places where I think it'd be good to keep an assertion, and a few places where it looks like the /* c8 ignore next */ magic comments aren't working as intended so maybe those can get a closer look.
There are a couple of deletions that don't look obviously deleteable to me, so would appreciate hearing your analysis.
| if (typeof monthCode !== 'string') { | ||
| throw new RangeErrorCtor( | ||
| `monthCode must be a string, not ${Call(StringPrototypeToLowerCase, Type(monthCode), [])}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Could be an assertion? assert(typeof monthCode === 'string'); Or maybe this one is fine without an assertion.
| while (compareISODateToLegacyDateRange(safeIsoDate) !== 0) { | ||
| numCycles++; | ||
| safeIsoDate = ES.AddDaysToISODate(isoDate, -numCycles * cycleDays * direction); | ||
| } |
There was a problem hiding this comment.
could be an assertion? assert(compareISODateToLegacyDateRange(safeIsoDate) === 0, "numCycles calculation should be correct")
| monthDayISOReferenceYear(/* monthCode, day */) { | ||
| /* c8 ignore next */ assertNotReached(`monthDayISOReferenceYear() should be implemented for ${this.id}`); | ||
| }, | ||
| monthDayISOReferenceYear(/* monthCode, day */) {}, |
There was a problem hiding this comment.
I would prefer to keep this as-is, to catch programmer errors. I thought the c8 ignore next comment would keep the assertion from showing up as a coverage gap, but if that's not working maybe we need to add a comment that ensures the whole function is not considered? (Since it's supposed to be overridden)
| reviseIntlEra(calendarDate /*, isoDate*/) { | ||
| let { era, eraYear } = calendarDate; | ||
| // Firefox 96 introduced a bug where the `'short'` format of the era | ||
| // option mistakenly returns the one-letter (narrow) format instead. The | ||
| // code below handles either the correct or Firefox-buggy format. See | ||
| // https://bugzilla.mozilla.org/show_bug.cgi?id=1752253 | ||
| if (era === 'b') era = 'bce'; | ||
| if (era === 'a') era = 'ce'; | ||
| return { era, eraYear }; | ||
| } |
There was a problem hiding this comment.
Yeah, I think it's good we can get rid of this. The bug was fixed in Firefox 106, released Oct '22.
| } else if (sign == -1) { | ||
| if (!(nudgeWindow.endEpochNs.leq(destEpochNs) && destEpochNs.leq(nudgeWindow.startEpochNs))) { | ||
| // Retry nudge window if it's out of bounds | ||
| nudgeWindow = ComputeNudgeWindow( | ||
| sign, | ||
| duration, | ||
| originEpochNs, | ||
| isoDateTime, | ||
| timeZone, | ||
| calendar, | ||
| increment, | ||
| unit, | ||
| true | ||
| ); | ||
| assert( | ||
| nudgeWindow.endEpochNs.leq(destEpochNs) && destEpochNs.leq(nudgeWindow.startEpochNs), | ||
| `${unit} was 0 days long` | ||
| ); | ||
| didExpandCalendarUnit = true; | ||
| } |
There was a problem hiding this comment.
This I'm not sure about. I still suspect there's a test case that could be written for it. (Although I believe it's been looked at before, unsuccessfully...)
At least we should change it into an assertion (else { assert(sign === 0); }? or is it the nudge window condition that should never be hit?) And do the same in the spec text. (But if we change the spec text, we should really be prepared to prove that it's impossible to write a test case that would hit this.)
| if (increment > 1) { | ||
| // If the estimate overshot the target, try again with a smaller increment | ||
| // in the reverse direction. | ||
| increment /= 2; |
There was a problem hiding this comment.
Removing this looks wrong at first glance — as if it might be just happenstance that the bisection never overshoots in any of our tests.
(if removing it is correct, then the loop can be simplified a bit more — it looks like sign = 0 can become break, and the comment above the loop needs to be updated since it's no longer a bisection)
| /* c8 ignore next */ assertNotReached( | ||
| `reference year ${startDateIso.year} should be correct for ${monthCode}-${day}` | ||
| ); |
There was a problem hiding this comment.
This seems like it should be OK to stay, the c8 ignore next comment should prevent it from showing as a coverage gap. (If that's not working, maybe there's some other way we need to format the magic comment?)
| reviseIntlEra(calendarDate, isoDate) { | ||
| const { era, eraYear } = calendarDate; | ||
| const { year: isoYear } = isoDate; | ||
| if (Call(ArrayPrototypeFind, this.eras, [(e) => e.code === era])) return { era, eraYear }; | ||
| return isoYear < 1 ? { era: 'bce', eraYear: 1 - isoYear } : { era: 'ce', eraYear: isoYear }; | ||
| } |
There was a problem hiding this comment.
I think we might still need this for browsers that don't have the CE/BCE eras in the Japanese calendar, but still are fairly recent.
| // The new object's cache starts with the cache of the old object | ||
| if (!OneObjectCache.getCacheForObject(this.id, isoAdded)) { | ||
| const newCache = new OneObjectCache(this.id, cache); | ||
| newCache.setObject(isoAdded); | ||
| } |
There was a problem hiding this comment.
It's not obvious to me why this can be removed (although I haven't managed to make a test case that hits it)
Thanks for taking a look! The analysis of these was based on nothing deeper than looking at the coverage and noticing that they appear unused. I'm happy to take a more conservative approach and replace some of the deleted bits with assertions. I agree that some of these unused blocks are large enough to be better viewed as missing tests than as dead code. I'll take care of some of the lower-hanging fruit and try to turn those larger bits into tests. Until then, I'll mark this as a draft PR. |
2 participants
After digging into code coverage following tc39/test262#4964, we found several unreachable code paths in the polyfill's non-ISO calendar and duration rounding logic, even after flossing for edge cases.
We verified this on Node only, so it's possible some of these paths are reachable in other engines. Happy to revert any individual removal if that turns out to be the case.