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: remove time check #4494

Closed
wants to merge 1 commit into from
Closed

test: remove time check #4494

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 31, 2015

test-child-process-fork-net2.js checks that things happen within
certain time constraints, thus doubling as a benchmark test in addition
to a functionality test.

This change removes the time check, as it was causing the test to fail
on SmartOS and Windows (and possibly elsewhere) when the tests were
run in parallel on CI. There is no guarantee that other tests won't
consume enough resources to slow this test down, so don't check the time
constraints (beyond the generous timeout that the test is given by
test.py in the first place, of course).

If we want to do benchmark/performance tests, we should keep them
separate from pure functionality tests. The time check may have been a
remnant of the distant past when Node.js was much slower. It predates
io.js

Ref: #4476

R=@jbergstroem ?

test-child-process-fork-net2.js checks that things happen within
certain time constraints, thus doubling as a benchmark test in addition
to a functionality test.

This change removes the time check, as it was causing the test to fail
on SmartOS and Windows (and possibly elsewhere) when the tests were
run in parallel on CI. There is no guarantee that other tests won't
consume enough resources to slow this test down, so don't check the time
constraints (beyond the generous timeout that the test is given by
test.py in the first place, of course).

If we want to do benchmark/performance tests, we should keep them
separate from pure functionality tests. The time check may have been a
remnant of the distant past when Node.js was much slower. It predates
io.js

Ref: nodejs#4476
@Trott Trott added the test Issues and PRs related to the tests. label Dec 31, 2015
@Trott
Copy link
Member Author

Trott commented Dec 31, 2015

@Trott
Copy link
Member Author

Trott commented Dec 31, 2015

One CI infra failure and one unrelated test failure on Raspberry Pi. CI looks good, I think.

@jbergstroem
Copy link
Member

Can you test on top of -J as well? I'll try to keep that PR rebased and fresh

@Trott
Copy link
Member Author

Trott commented Dec 31, 2015

@Trott
Copy link
Member Author

Trott commented Dec 31, 2015

@jbergstroem Test is now passing on SmartOS in parallelized CI. A bunch of other unrelated failures still, of course.

@mscdex mscdex added the child_process Issues and PRs related to the child_process subsystem. label Dec 31, 2015
@jbergstroem
Copy link
Member

@Trott: Smartos looks pretty messed up (still going?!) in the above build. I think the problem might be that -J gets # cores from multiprocessing which in above case would be 48 (its crazy, yes). I think this needs to be controlled through JOBS similar to how we manage gmake.

@Trott
Copy link
Member Author

Trott commented Dec 31, 2015

Heh, the test in this PR is run pretty early on in the suite (71st test) so I saw that it passed and didn't look again. That's...impressive...that it's still running....

@jbergstroem
Copy link
Member

@Trott when you have a moment, can you rebase on -J again to give it a go? Should zero out parallelism-related issues on SmartOS at least.

@cjihrig
Copy link
Contributor

cjihrig commented Dec 31, 2015

Code change LGTM FWIW

@Trott
Copy link
Member Author

Trott commented Dec 31, 2015

@jbergstroem
Copy link
Member

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Jan 3, 2016
test-child-process-fork-net2.js checks that things happen within
certain time constraints, thus doubling as a benchmark test in addition
to a functionality test.

This change removes the time check, as it was causing the test to fail
on SmartOS and Windows (and possibly elsewhere) when the tests were
run in parallel on CI. There is no guarantee that other tests won't
consume enough resources to slow this test down, so don't check the time
constraints (beyond the generous timeout that the test is given by
test.py in the first place, of course).

If we want to do benchmark/performance tests, we should keep them
separate from pure functionality tests. The time check may have been a
remnant of the distant past when Node.js was much slower. It predates
io.js

Ref: nodejs#4476
PR-URL: nodejs#4494
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@Trott
Copy link
Member Author

Trott commented Jan 3, 2016

Landed in 9a6cfce

@Trott Trott closed this Jan 3, 2016
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
test-child-process-fork-net2.js checks that things happen within
certain time constraints, thus doubling as a benchmark test in addition
to a functionality test.

This change removes the time check, as it was causing the test to fail
on SmartOS and Windows (and possibly elsewhere) when the tests were
run in parallel on CI. There is no guarantee that other tests won't
consume enough resources to slow this test down, so don't check the time
constraints (beyond the generous timeout that the test is given by
test.py in the first place, of course).

If we want to do benchmark/performance tests, we should keep them
separate from pure functionality tests. The time check may have been a
remnant of the distant past when Node.js was much slower. It predates
io.js

Ref: nodejs#4476
PR-URL: nodejs#4494
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
MylesBorins pushed a commit that referenced this pull request Jan 13, 2016
test-child-process-fork-net2.js checks that things happen within
certain time constraints, thus doubling as a benchmark test in addition
to a functionality test.

This change removes the time check, as it was causing the test to fail
on SmartOS and Windows (and possibly elsewhere) when the tests were
run in parallel on CI. There is no guarantee that other tests won't
consume enough resources to slow this test down, so don't check the time
constraints (beyond the generous timeout that the test is given by
test.py in the first place, of course).

If we want to do benchmark/performance tests, we should keep them
separate from pure functionality tests. The time check may have been a
remnant of the distant past when Node.js was much slower. It predates
io.js

Ref: #4476
PR-URL: #4494
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
MylesBorins pushed a commit that referenced this pull request Jan 14, 2016
test-child-process-fork-net2.js checks that things happen within
certain time constraints, thus doubling as a benchmark test in addition
to a functionality test.

This change removes the time check, as it was causing the test to fail
on SmartOS and Windows (and possibly elsewhere) when the tests were
run in parallel on CI. There is no guarantee that other tests won't
consume enough resources to slow this test down, so don't check the time
constraints (beyond the generous timeout that the test is given by
test.py in the first place, of course).

If we want to do benchmark/performance tests, we should keep them
separate from pure functionality tests. The time check may have been a
remnant of the distant past when Node.js was much slower. It predates
io.js

Ref: #4476
PR-URL: #4494
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
test-child-process-fork-net2.js checks that things happen within
certain time constraints, thus doubling as a benchmark test in addition
to a functionality test.

This change removes the time check, as it was causing the test to fail
on SmartOS and Windows (and possibly elsewhere) when the tests were
run in parallel on CI. There is no guarantee that other tests won't
consume enough resources to slow this test down, so don't check the time
constraints (beyond the generous timeout that the test is given by
test.py in the first place, of course).

If we want to do benchmark/performance tests, we should keep them
separate from pure functionality tests. The time check may have been a
remnant of the distant past when Node.js was much slower. It predates
io.js

Ref: #4476
PR-URL: #4494
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants