moment / moment-timezone

Timezone support for moment.js
momentjs.com/timezone
MIT License
3.83k stars 838 forks source link

Test failure due to rounding error #1096

Open NightTsarina opened 9 months ago

NightTsarina commented 9 months ago

Hi,

While building moment.js 0.5.44 for Debian, where data files are re-generated and tests run, I get a rounding error for Africa/Lome:

$ nodeunit tests/zones/africa/lome.js 

lome.js
✖ Africa/Lome - 1892

Assertion Message: 1892-12-31T23:55:07+00:00 should be -4.866666666666666 minut
es offset in LMT
AssertionError: 4.866666666666667 == 4.866666666666666
    at Object.equal (/usr/share/nodejs/nodeunit/lib/types.js:83:39)
    at testYear (/build/moment-timezone.js-0.5.44+dfsg/tests/helpers/helpers.js
:34:8)
    at Object.<anonymous> (/build/moment-timezone.js-0.5.44+dfsg/tests/helpers/
helpers.js:102:4)
    at Object.<anonymous> (/usr/share/nodejs/nodeunit/lib/core.js:236:16)
    at Object.<anonymous> (/usr/share/nodejs/nodeunit/lib/core.js:236:16)
    at /usr/share/nodejs/nodeunit/lib/core.js:236:16
    at Object.exports.runTest (/usr/share/nodejs/nodeunit/lib/core.js:70:9)
    at /usr/share/nodejs/nodeunit/lib/core.js:118:25
    at /usr/share/javascript/async/async.js:665:13
    at iterate (/usr/share/javascript/async/async.js:149:13)

✔ Africa/Lome - guess:by:offset
✔ Africa/Lome - guess:by:abbr

FAILURES: 1/8 assertions failed (79ms)

I cannot tell if this is a problem in the extraction from the tzdata database, or in the tests themselves, but for now I have patched the tests to only consider 4 decimal digits in this comparison:

--- a/tests/helpers/helpers.js
+++ b/tests/helpers/helpers.js
@@ -14,6 +14,11 @@
    }
 }

+function nRound(num, digits) {
+   var mult = 10 ** digits;
+   return Math.round((num + Number.EPSILON) * digits) / digits;
+}
+
 /**
  * Runs tests for a specific year
  */
@@ -31,7 +36,7 @@
        offset = expected[i][3];
        m      = moment(date).tz(name);
        test.equal(m.format("HH:mm:ss"), time, date + ' should be ' + time + ' ' + abbr);
-       test.equal(getUTCOffset(m), -offset, date + ' should be ' + offset + ' minutes offset in ' + abbr);
+       test.equal(nRound(getUTCOffset(m), 4), nRound(-offset, 4), date + ' should be ' + offset + ' minutes offset in ' + abbr);
        test.equal(m.zoneAbbr(), abbr, date + ' should be ' + abbr);
    }
gilmoreorless commented 8 months ago

That's a fascinating bug — aren't floating point errors fun? What gets me is that the data/test files in the latest release (0.5.45) were built using GitHub Actions on an Ubuntu machine: https://github.com/actions/runner-images/blob/ubuntu22/20240126.1/images/ubuntu/Ubuntu2204-Readme.md

I'll take a look at adding in your suggested rounding, because this is likely to pop up again sometime.

NightTsarina commented 8 months ago

That's wild! I don't fully understand how those values are obtained, so I can't help much trying to narrow down the bug, but the rounding seems like a reasonable compromise...