Times: JS: Remove implicit UTC convesion.

[moigagoo]

The conversion would produce incorrect timestamps during parsing.
See https://forum.nim-lang.org/t/2876, point 2.

[dom96]

I asked for tests in the last PR (#5581). I will ask again because I think we run the risk of continuously changing the times code without knowing what we are breaking.

[moigagoo]

Yeah, I'm totally with you on the necessity of tests. In fact, I was very much puzzled by Araq accepting the PR without the tests.

I just wanted to file the change right away so that I wouldn't have to remember to submit it. I am not hoping the PR will be merged until I provide the tests. If it's bad/annoying/distracting to post PRs this way, please feel free to delete this one. I'll resubmit it later with tests (when I figure out how to write and run them with Koch).

[moigagoo]

In fact I was just going to submit another untested PR (moigagoo@bef86f5). I'll hold it until you tell me what the right thing to do is.

[stisa]

If you place a .nim file that starts with t in tests/js it will be run with the test suite, using nodejs.
To run only the tests for js, run koch tests --targets:js c js, it may take a while as it builds the compiler and then runs all tests in tests/js, with something like nim js -d:nodejs -r <test.nim> .
As for how to write them, someone recently suggested to use doAssert, which is an assert that is run even when compiling with -d:release ( tests are run both with and without release, so assert is not enough ). Example

(I'm not an expert at this, so dom can probably add to this.)

[moigagoo]

@stisa Thanks for the info! I actually figured it out myself. I was having trouble writing the actual test, namely making it fail.

It turns out if the first line of a test is a comment, it always passes no matter what content it has. I spent about an hour to realize that.

Like, this always passes:

# a comment
discard """
  action: run
"""

doAssert false == true

And this always fails:

discard """
  action: run
"""

doAssert false == true

[moigagoo]

OK, the previous examples work like they should, but here's the actual tests that passes when it should not:

# test times module with JS backend
discard """
  action: run
"""

import times

# $ date --date='@2147483647'
# Tue 19 Jan 03:14:07 GMT 2038

block yeardayTest:
  # check

  doAssert fromSeconds(2147483647).getGMTime().yearday == 0

This one fails as it should:

discard """
  action: run
"""

import times

# $ date --date='@2147483647'
# Tue 19 Jan 03:14:07 GMT 2038

block yeardayTest:
  # check

  doAssert fromSeconds(2147483647).getGMTime().yearday == 0

[moigagoo]

I have no idea why this happens. @dom96 maybe you could tell?

UPD: Now this is complete nonsense. This comment breaks the test: # test times module with js backend, but this one doesn't: # test times module with js. Is the first one too long? What's going on here?

[Araq]

@moigagoo the discard section must be within the first 40 chars of the file or something like that. Stupid testament glitch -- consider it fixed, but for now always have the discard on the very first line.

[moigagoo]

@Araq Thanks a lot for the clarification!

[moigagoo]

Added tests for #5614, #5616, and #5581.

[moigagoo]

The tests do pass locally. Could you rerun the build to see if the checks pass now?

[Araq]

FAIL: ttimes.nim -d:nodejs
Test "tests/js/ttimes.nim" in category "js"
Failure: reExitcodesDiffer
Expected:
exitcode: 0
Gotten:
exitcode: 1
Output:
Traceback (most recent call last)
system.raiseAssert
FAIL: ttimes.nim -d:nodejs -d:release
Test "tests/js/ttimes.nim" in category "js"
Failure: reExitcodesDiffer
Expected:
exitcode: 0
Gotten:
exitcode: 1
Output:

Well your tests fail.

[moigagoo]

@Araq thanks for the update. I'll investigate.

[moigagoo]

@Araq While fixing the test I've encountered a problem. I hope you could help me.

Proc toTime is currently written in a way that dumps timezone information from the incoming timeInfo:

  proc toTime*(timeInfo: TimeInfo): Time =
    result = internGetTime()
    result.setMinutes(timeInfo.minute)
    result.setHours(timeInfo.hour)
    result.setMonth(ord(timeInfo.month))
    result.setFullYear(timeInfo.year)
    result.setDate(timeInfo.monthday)
    result.setSeconds(timeInfo.second)

Since JS can produce Date objects from strings, the better way to do the conversion seems to be to call newDate($timeInfo) instead, especially since newDate(value: string) is already defined:

  proc newDate(value: cstring): Time {.importc: "new Date".} # fix type: string → cstring
...
  proc toTime*(timeInfo: TimeInfo): Time = newDate($timeInfo)

However, for some reason, $timeInfo does not return the expected string in "yyyy-MM-dd'T'HH-mm-sszzz" format, but the string representation of the JS object, something like this: "(second: 56, minute: 34, hour: 13, monthday: 21, month: mMar, year: 2017, weekday: dTue, yearday: 79, isDST: false, timezone: -14400)".

Also, I can't use format to normalize timeInfo to the desired format.

I feel that the problem is that $ and format are not defined for timeInfo at the time when toTime is executed, so the fallback versions are executed. Although I'm not sure, so please clarify that bit to me if you don't mind helping newbies.

I could, of course, build the string manually (e.g. "$#-$#-$#T$#:$#:$#" % [timeInfo.year, ...]), but this is ugly and I'm sure there's a better way.

[Araq]

I don't know. To test your hypothesis, move the declarations around as you see fit.

[moigagoo]

According to the build logs, the tests have actually passed, but the build still failed with sh: 1: gtar: not found. Doesn't seem related to my changes.

[dom96]

The job exceeded the maximum time limit for jobs, and has been terminated.

Travis is really failing us here. We seem to be hitting some sort of bug in it.

[moigagoo]

@dom96 Could this be related?

[dom96]

Perhaps. I restarted it in any case, we'll see if it succeeds this time.

[moigagoo]

Yay! It passed!

[dom96]

Brilliant! Merging :)