test/timers/

[Trott]

test-timers-reliability.js is failing consistently on OS X (and probably elsewhere). Is this something that needs to be addressed or is the test obsolete?

CC=@Fishrock123

It isn't caught on CI because this particular test is not run on CI. I don't know why it was excluded. Might have been placed in a directory that was appropriate for an older version of Node.js and never moved to test/parallel or test/sequential.

Here is the output I get on OS X:

/Applications/Xcode.app/Contents/Developer/usr/bin/make --directory=tools faketime
cd /Users/trott/io.js/tools/faketime && \
    git checkout master && \
    PREFIX=/Users/trott/io.js/tools/faketime/src make
Already on 'master'
Your branch is up-to-date with 'origin/master'.
/Applications/Xcode.app/Contents/Developer/usr/bin/make -f Makefile.OSX -C src all
make[3]: Nothing to be done for `all'.
/usr/bin/python tools/test.py --mode=release timers
=== release test-timers-reliability ===                    
Path: timers/test-timers-reliability
Assertion failed: (cb_v->IsFunction()), function MakeCallback, file ../src/async-wrap-inl.h, line 107.
Caught Abort trap: 6
Command: tools/faketime/src/faketime --exclude-monotonic -f "2014-07-21 09:00:00" out/Release/node /Users/trott/io.js/test/timers/test-timers-reliability.js
[00:07|% 100|+   0|-   1]: Done                           
make: *** [test-timers] Error 1

[Fishrock123]

It'll take a look at it soon(tm), I was actually looking through some of these tests recently.

[misterdjules]

test-timers-reliability.js is failing consistently on OS X (and probably elsewhere). Is this something that needs to be addressed or is the test obsolete?

test-timers-reliability.js was added by befbbad, which made timers use monotonic time instead of wall clock time to determine if they had expired. The goal of that test was to make sure no future change would make timers use a different type of clock. So I don't think that this test is obsolete.

It isn't caught on CI because this particular test is not run on CI. I don't know why it was excluded.

This test was not excluded, it was just never run as part of the tests suite run by the CI platform. The main reason why it wasn't added to the tests run by the CI platform at the time was that it wasn't running on all platform on which our CI tests' were running.

Here is the output I get on OS X:

The failure on OSX can be fixed by this small patch:

diff --git a/test/timers/test-timers-reliability.js b/test/timers/test-timers-reliability.js
index b433246..c1fefba 100644
--- a/test/timers/test-timers-reliability.js
+++ b/test/timers/test-timers-reliability.js
@@ -32,7 +32,7 @@ var intervalFired = false;
  */

 var monoTimer = new Timer();
-monoTimer.ontimeout = function() {
+monoTimer[Timer.kOnTimeout] = function() {
     /*
      * Make sure that setTimeout's and setInterval's callbacks have
      * already fired, otherwise it means that they are vulnerable to

But that wouldn't solve the broader issue of having that test run on all platforms. I think we should look into that again, as this test could catch an important regression.

[Trott]

We have lots of tests that check to see what platform they are running on. If they are running on a platform where the test does not apply for whatever reason, the test prints TAP output indicating the test is to be skipped and then exits. We could likely do that for this test and then move it to test/parallel.

[misterdjules]

I agree that enabling these tests as part of the CI tests on the platforms on which they currently run fine seems to only have benefits.

Now, here are a few important notes:

1. The tests seem to run fine only on OS X and Linux. They don't work on SmartOS for a reason that I couldn't understand fully. Basically, faketime crashes when invoked the way that test invokes it due to the thread local storage implementation of SmartOS. I haven't been able to find the root cause, a fix or even a work around. They also don't work on Windows because faketime doesn't run on Windows. In other words, for now we'd enable the test on Linux and OSX only. We'd have to test that on FreeBSD and other OSes that are now available on our CI platform.
2. At least on OS X, one of the latest changes to libfaketime prevents libfaketime from building correctly. This can be easily solved by a small change to tools/Makefile, but it will not work with current nodejs/node's master.
3. The test relies on a setup step that clones the libfaketime repository. I don't think any other test has that kind of requirement. We might want to include that dependency in the tree, or we'll have to find another solution if we want to keep using libfaketime.
4. test/timers/testcfg.py was written to make evert test in test/timers to be run via faketime, which basically override all time-related library calls so that the tests are actually meaningful. So I'm not sure we can move individual tests from test/timers to test/parallel without refactoring that part too.

[Trott]

Perhaps a good solution would be to put faketime into the tools directory rather than cloning it via git so we can make sure we have a version that compiles in the relevant places?

I played around with some changes to make it at least run under Linux on CI, but the results were mixed. I suspect we can get it going with just a bit more effort, though.

[misterdjules]

Perhaps a good solution would be to put faketime into the tools directory rather than cloning it via git so we can make sure we have a version that compiles in the relevant places?

Just to make sure we're on the same page, including faketime's source in node's tree is not necessary to make sure we have a version of faketime that builds on all supported platforms. For that we can just change tools/Makefile like following:

diff --git a/tools/Makefile b/tools/Makefile
index d627c14..5e68bb7 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -1,6 +1,6 @@
 FAKETIME_REPO       := git://github.com/wolfcw/libfaketime.git
 FAKETIME_LOCAL_REPO := $(CURDIR)/faketime
-FAKETIME_BRANCH     := master
+FAKETIME_COMMIT     := cae9387908c7cd7ed1c7634ddb6fc72ed370261e
 FAKETIME_BINARY     := $(FAKETIME_PREFIX)/bin/faketime

 .PHONY: faketime
@@ -12,8 +12,8 @@ clean:

 $(FAKETIME_BINARY): $(FAKETIME_LOCAL_REPO)
        cd $(FAKETIME_LOCAL_REPO) && \
-       git checkout $(FAKETIME_BRANCH) && \
-       PREFIX=$(FAKETIME_LOCAL_REPO)/src make
+       git checkout $(FAKETIME_COMMIT) && \
+       make PREFIX=$(FAKETIME_LOCAL_REPO)/src LIBDIRNAME=''

 $(FAKETIME_LOCAL_REPO):
        git clone $(FAKETIME_REPO) $(FAKETIME_LOCAL_REPO)

Where cae9387908c7cd7ed1c7634ddb6fc72ed370261e is a libfaketime commit that is known to work across most supported node platforms (but not all as mentioned before). At that point using git submodules might be easier too.

Moving libfaketime's source in tree would however have the additional advantage of not requiring any separate download or usage of git submodules.

I haven't made up my mind on what would be my preferred solution yet.

[Fishrock123]

Sounds like you both understand more about what's going on than I do.

[Trott]

Do we currently go out to the network for anything in CI other than the source for Node.js itself? If not, then I'd be inclined to put it in tools or deps or something. But if we already do git clone a bunch of times in CI for other things, then hey, what's one more? /cc @nodejs/build

[Fishrock123]

@misterdjules is there a simpler way we can induce non-monotonic clock drift without using another dependency? (Via an addon test maybe?)

[misterdjules]

@Fishrock123 It would be possible to roll our own implementation, but I'm not sure it would make things any easier, as we'd still need to do the work to support SmartOS, Windows and other platforms. The advantage of using libfaketime is that some of that work is already done, and the project is well maintained.

I'm not aware of any other way to reliably fake non-monotonic clock drift that would be practical for this specific use case, but don't let this prevent you from exploring, I may be missing something.