Test via iojs throws but python/make do not report error

[Trott]

out/Release/iojs test/parallel/test-http-get-pipeline-problem.js throws:

Error: ENOENT: no such file or directory, open '/Users/trott/io.js/test/tmp/0.jpg

But python tools/test.py -v --mode=release parallel/test-http-get-pipeline-problem completes successfully.

Is this a bug or do I misunderstand how the tests are supposed to work?

[Trott]

out/Release/iojs test/parallel/test-http-get-pipeline-problem.js returns error code 1, so it's not just printing something but really honestly returning an error code. Checked with:

out/Release/iojs test/parallel/test-http-get-pipeline-problem.js || echo $?

[brendanashworth]

I've come across this before. I think that the python test suite creates the temporary directory (found in test/common.js?), but the individual test suites won't create it.

[Trott]

Yes, that's exactly right. The python test harness (or whatever the right word is) is creating the tmp dir and the command line is failing if I don't create the tmp dir it expects. Should a "create tmp dir if it doesn't exist" bit be added to the test file? Or is this just sort of a known gotcha and it should be left as is?

[Trott]

Not seeing any code anywhere in any of the other tests to create the temp dir if it doesn't exist, so I'm guess this is just something you just sort of have to know.

[Trott]

Oh, wait, that code could be stuck inside of common.js...

[brendanashworth]

Well, putting it in common.js would be best, but we don't want to remove it and add it every time a test is run (x1000 in the real test suite). I'm not really sure the best way to approach that, while maintaining a clean test run each time.

[Trott]

Even easier might be to check in test/tmp (since that is what the directory will always be called outside of the python multiple threads thing). Git won't allow empty directories, but we could add a .gitignore file. Not sure if there's a test that will end up deleting such a file, though...

I wouldn't want common.js to add it and remove it after every test, but just add it if it's not already there. But still, all those existsSync() calls can add up. I'll try it to see how much of a cost there is, and then probably abandon the idea once I see...

[brendanashworth]

Perhaps use a Getter for the fixtures directory, that's been done in the past for other things there.

[jbergstroem]

At the moment we kind of expect tests to be run through test.py. It creates one temp dir per parallel runner and does some other stuff. I'm not really against adding more to common.js, but I hope somewhere down the line we can move to a pure javascript runner and improve test information by switching fully to tap(3).

[Trott]

Adding this to common.js does not seem to noticeably impact the run time of make test-parallel:

if (! fs.existsSync(exports.tmpDir)) {
  fs.mkdirSync(exports.tmpDir);
}

Can someone do more rigorous benchmarks (or point me to docs or something on how to do it)? I'll stick it in a PR for now.

[chrisdickinson]

Alternatively, we could have the python test runner set an environment variable that common.js checks for the absence of to see if it needs to be responsible for creating the temporary directory.

[bnoordhuis]

@Trott if (!fs.existsSync(exports.tmpDir)) fs.mkdirSync(exports.tmpDir); is race-y when you run tests in parallel. It's better to just call fs.mkdirSync() and handle the EEXIST error.

I don't think that's enough, though. Tests don't just expect that the temp directory is there but that it's empty as well. In other words, if the directory exists, it needs to be cleaned out. And because it can contain subdirectories, you're going to have to do a recursive walk of the directory tree.

The test runner already handles all of that so it's basically duplicating existing functionality.

[silverwind]

I'm -1 on changing tests specifically to run standalone. It is documented that single tests should run through test.py. As @jbergstroem outlined, I think these kind of changes are best done in batch if we are to switch to a new test runner.

[Trott]

@silverwind Below the instructions for running tests through test.py, it says:

You can run tests directly with iojs:

    $ iojs ./test/parallel/test-streams2-transform.js

That will fail in many cases if the user does not manually create ./test/tmp. Perhaps the easiest solution is to add a note to that effect? Or even just remove the recommendation for running via iojs altogether? (Although I would want it documented how to make console.error() output visible when run via python.)

[silverwind]

Hmm, not sure. Could update the doc to indicate that most tests can be run directly, but it'd be somewhat nice if we find a solution to fix this. I haven't looked at it yet, but recently encountered this:

$ iojs test/parallel/test-tls-securepair-server.js
internal/child_process.js:273
  var err = this._handle.spawn(options);
                         ^
TypeError: Bad argument

Not sure what's going on there exactly.

[Trott]

So, just to make sure I'm not making naïve assumptions:

• Python code that handles temp directory creation and cleanup is straightforward. Hooray.
• However, io.js does not have an easy equivalent of rm -rf like Python does. That's why rimraf exists.
• Am I correct to imagine that requiring an npm module (in this case, rimraf) for the tests is a non-starter, even if we do something like check it in to the io.js repo so it doesn't need to be installed with npm as part of the build/test process?
• Otherwise, hello directory tree walking and all the likely OS-specific wonky edge cases that probably entails. Not a ridiculous amount of work, especially with rimraf code to refer to. But given the duplication of rimraf functionality, kind of feels like an unnecessary burden in terms of maintenance cost. And would we really want to trade the simple Python code for this?
• But seriously, the individual tests should just work from the command line without the Python wrapper. So... I dunno...

[silverwind]

While I agree that the goal should be to have each test run as standalone, I don't think it's beneficial to have the same duplicate code in test.py and common.js. If you can move the temp dir creation and deletion to common.js while making sure it ony runs once per test run, I could see it being approved.

Having rimraf there as a dependency for tests is a bit suboptimal as it can possibly introduce issues of its own, not sure on that one.

[Trott]

So, this is kind of interesting, I think...

While putting together a proof-of-concept (or, as it may turn out to be, a disproof-of-concept, but either way...), I was wondering why the Python code deleted and re-created the temp directory right before and right after each test. That seemed unnecessary.

So I wrote my code to only deal with the temp directories before each test.

Weirdly, this causes test/sequential/fs-watch-recursive to fail, but only when it is run as part of the test suite (and only on OS X because that's the only place recursive watch() is supported).

It turns out that stale watch events from the previous test (test/sequential/fs-watch) leak into that test! And so the Python code (inadvertently?) provides a sort of work around by scrubbing the directory twice before launching the test.

This is weird to me, and I'm inclined to rewrite one or both tests so that they don't both use a directory named testsubdir and therefore don't risk getting each other's events.

[Trott]

Pushed a new commit to #1877. See notes at #1877 (comment).

[Trott]

#1877 has landed, closing this.