-
Notifications
You must be signed in to change notification settings - Fork 893
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
Refactor time module and fix its tests #1233
Refactor time module and fix its tests #1233
Conversation
Further info re Travis build failure: A local branch merging this PR and #1232 builds and passes all tests: % git checkout -b test-1232-and-1233 use-terser
% git merge --no-edit --no-ff refactor-time-module-and-fix-its-tests
% rm -r node_modules
% npm install && npm run build && npm run test
[...]
test/unit/[...]
Passed: 462 Failed: 0
test/unit3/[...]
Passed: 938 Failed: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - works for my timezone in KL, Malaysia which has always failed.
For consistency with how time.timezone is computed.
Code taken from existing localtime_f() and mod.gmtime(), changed slightly to take arg saying whether to treat input as UTC. squash! from_seconds(): Add JS helper function
Also fix copy/paste typo so gmtime() uses correct function name in its call to pyCheckArgsLen().
Test functions asctime ctime gmtime localtime mktime strftime strptime with regard to raising errors if an incorrect number of arguments passed. Currently, the tests fail for: asctime ctime mktime This will be addressed in the next commit.
Fixes remaining tests added previously.
1812eea
to
450f442
Compare
This pr has caused a bug - |
Apologies. I'll have a look at that now. |
I've just created #1251. Sorry for overlooking this. |
Follow-up to the aside in #1231 regarding the failed
TimeTestCase.test_strftime
test.If I'm reading it right, Skulpt's implementation of
time.timezone
uses the offset as of 2002. I'm in Ireland and the history of its civil timekeeping means that its timezone offset was different in 1970 compared to 2002. It's likely that developers in other timezones weren't seeing this failure. This PR updates the test to use a moment in 2002, for consistency with howtimezone
is computed. I've checked it under timezonesEurope/Dublin
,America/Denver
,Europe/Istanbul
.This PR also extracts a function from near-duplicated code between Skulpt's
localtime()
andgmtime()
implementations, and fixes a minor typo.