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

Test failure due to rounding error #1096

Open
NightTsarina opened this issue Jan 25, 2024 · 2 comments
Open

Test failure due to rounding error #1096

NightTsarina opened this issue Jan 25, 2024 · 2 comments

Comments

@NightTsarina
Copy link

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 gilmoreorless transferred this issue from moment/moment Feb 5, 2024
@gilmoreorless
Copy link
Member

gilmoreorless commented Feb 18, 2024

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
Copy link
Author

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...

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

No branches or pull requests

2 participants