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: split simple into io/simple, run in parallel #172

Closed
wants to merge 8 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Dec 16, 2014

Before:

[02:04|% 100|+ 786|-   0]: Done

After:

[01:44|% 100|+ 786|-   0]: Done

Fix #139

@indutny
Copy link
Member Author

indutny commented Dec 16, 2014

cc @bnoordhuis @piscisaureus

I'm still +1 in a favor of calling it parallel and sequential and moving more tests to parallel.

@indutny
Copy link
Member Author

indutny commented Dec 16, 2014

Here is a test split at the moment:

~/Code/indutny/node $ ls test/io/ | wc -l
     541
~/Code/indutny/node $ ls test/simple/ | wc -l
     232

I think it could be closer to 50%

Support mixed parallel/sequential tests. Merge the `io` and `simple
message` test runs together in a Makefile
@chrisdickinson
Copy link
Contributor

In general, LGTM. A few thoughts:

  1. I'd suggest putting README.md files in the two test directories ({simple,io}/) that state clearly what to put in each test dir, and why (if for no other reason than to have a place to point to when reviewing PRs!)
  2. It would be interesting to see if we could parallelize more of the I/O tests by (ab)using common.PORT. If it turns out to be worthwhile, it'd probably be best to use the parallel / sequential nomenclature for the directory names.
  3. This is probably just my inner Python programmer talking, but I am immediately suspicious of Python's threading; specifically how much parallelism it's actually delivering here. It could just be superstition talking, though :)

@@ -90,7 +90,7 @@ distclean:
-rm -rf node_modules

test: all
$(PYTHON) tools/test.py --mode=release simple message
$(PYTHON) tools/test.py --mode=release simple message io -J
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is -J a typo? It's -j <jobs>, isn't it?

@indutny
Copy link
Member Author

indutny commented Dec 16, 2014

New test running time will make you cry, guys:

[00:32|% 100|+ 786|-   0]: Done

@indutny
Copy link
Member Author

indutny commented Dec 16, 2014

@bnoordhuis: btw, -J is not a typo.

@piscisaureus
Copy link
Contributor

Windows result:

running 'python tools/test.py --mode=release simple message'
[00:04|% 100|+  19|-   0]: Done

IOW, only the message tests are being run.

@piscisaureus
Copy link
Contributor

With a trivial fix (updating https://github.com/iojs/io.js/blob/165b70f146e163b82a09bb869463708516c08cf6/vcbuild.bat#L166):

[01:26|% 100|+ 785|-   1]: Done

That's great! I know it may not look so great to y'all spoiled unix users but that's 4 times faster than I'm used to.

Two small issues turned up.

1: I got a lot of these:

os.unlink() [Error 32] The process cannot access the file because it is being us
ed by another process: 'c:\\users\\bertbe~1\\appdata\\local\\temp\\tmpnwfzkp'

That's the test runner failing to delete a temp file. It doesn't cause any issues but it'd be nice to find out why it happens and if we should silence the error.

2: test-tls-server-verify timed out

That test used to time out a lot but I hadn't seen any failures recently.

@bnoordhuis
Copy link
Member

Nice, LGTM. Just curious, is there a reason you had to change tests that bind to a random port?

@chrisdickinson
Copy link
Contributor

LGTM. 👏

@bnoordhuis
Copy link
Member

However... I do see a number of failures on the CI.

@indutny
Copy link
Member Author

indutny commented Dec 17, 2014

@bnoordhuis: Gosh, this is what I was fighting all day, and basically the reason why I was changing tests from .bind(0) to normal port. I wonder if I have missed some tests.

@indutny
Copy link
Member Author

indutny commented Dec 17, 2014

@bnoordhuis seems to be fixed by latest commit: https://jenkins-node-forward.nodesource.com/job/iojs+any-pr+multi/

@bnoordhuis
Copy link
Member

LGTM, although I don't understand why changing from a random port to common.PORT would fix tests.

@indutny
Copy link
Member Author

indutny commented Dec 17, 2014

@bnoordhuis it looks like we was running the CI job in a wrong way. Also, the random port thing may conflict with common.PORT, as it gets a bit higher on higher id threads.

indutny added a commit that referenced this pull request Dec 17, 2014
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #172
Fix: #139
indutny added a commit that referenced this pull request Dec 17, 2014
* Allow running tests in mixed parallel/sequential modes
* Add -J flag for running tests on all available CPUs
* Support TEST_THREAD_ID in test/common.js and use it for tmpDir and PORT
* make: use -J flag

Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #172
Fix: #139
@indutny
Copy link
Member Author

indutny commented Dec 17, 2014

Landed in 0e19476 and b7d2478, thank you everyone!

@indutny indutny closed this Dec 17, 2014
@indutny indutny deleted the feature/io-tests branch December 17, 2014 13:46
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

Successfully merging this pull request may close these issues.

4 participants