test: remove excessively poor performance #2410
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
String concatenation in the assert messages has drastic impact on test
runtime. Removal of these messages is unlikely to affect debugging if
any breaking changes are made.
Previous time to run:
$ time ./iojs test/parallel/test-stringbytes-external.js
real 0m2.321s
user 0m2.256s
sys 0m0.092s
With fix:
$ time ./iojs test/parallel/test-stringbytes-external.js
real 0m0.518s
user 0m0.508s
sys 0m0.008s
Member
|
LGTM |
Contributor
Author
Member
|
lgtm, nice find, can you speed up MOAR TESTS? |
Member
|
I feel like assert messages should probably just be code comments in most cases. If you see it, it's because bad stuff happened. If bad stuff happened, you are going to look at the code. If you look at the code, you are going to see the comments that explain why it was making that assertion. |
Contributor
|
LGTM and what @Qard said makes sense. |
Member
Closed
Contributor
|
cc @trevnorris, ci looks good. LGTM. |
Contributor
Author
|
Thanks for the reminder. Had forgotten about this one. Landed in bfb5a58eab3. |
Member
|
Holy crap. Keep em comin' @trevnorris :) |
trevnorris
added a commit
that referenced
this pull request
Sep 1, 2015
String concatenation in the assert messages has drastic impact on test
runtime. Removal of these messages is unlikely to affect debugging if
any breaking changes are made.
Previous time to run:
$ time ./iojs test/parallel/test-stringbytes-external.js
real 0m2.321s
user 0m2.256s
sys 0m0.092s
With fix:
$ time ./iojs test/parallel/test-stringbytes-external.js
real 0m0.518s
user 0m0.508s
sys 0m0.008s
PR-URL: #2410
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
trevnorris
referenced
this pull request
Sep 1, 2015
String concatenation in the assert messages has drastic impact on test
runtime. Removal of these messages is unlikely to affect debugging if
any breaking changes are made.
Previous time to run:
$ time ./iojs test/parallel/test-stringbytes-external.js
real 0m2.321s
user 0m2.256s
sys 0m0.092s
With fix:
$ time ./iojs test/parallel/test-stringbytes-external.js
real 0m0.518s
user 0m0.508s
sys 0m0.008s
PR-URL: #2544
Reviewed-By: trevnorris - Trevor Norris <[email protected]>
Reviewed-By: thefourtheye - Sakthipriyan Vairamani <[email protected]>
Contributor
Author
|
Correction. Actual commit is 37ee43e. The |
Contributor
|
Here's what happened: #2544 (comment) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
String concatenation in the assert messages has drastic impact on test
runtime. Removal of these messages is unlikely to affect debugging if
any breaking changes are made.
Previous time to run:
With fix:
R=@bnoordhuis