test: remove test repetition #1874
Conversation
|
This iteration seems somewhat intentional, given this change: e014643 cc: @trevnorris |
|
I can only infer the intention of this test, as the commit messages tell us nothing. That is, to ensure that writing to a buffer slice many times does not cause it to overrun the internal bounds of the buffer. /cc @bnoordhuis Have any recollection of why the test was written this way? |
|
No specific recollection but I think the general idea is like you say, to shake out subtle memory corruption bugs. Updating the test doesn't bother me but neither does not updating the test. |
|
FWIW: on armv7 it only seems to take 1.6s with the potentially unnecessary repetition. |
|
Guess this is a case of 'if it aint broke don't fix it'. The argument about the unused variable is also invalid, as it is used to run the loop itself 😉 |
|
Can we maybe add a comment to the code to explain this then? |
|
@dcousens Pull Request welcome, no one would object that. |
|
I've gone over this in detail, along with the commit where it was originally implemented. I'm fairly confident what these tests were supposed to be doing aren't done correctly. Since buffer slice operates like array slice it won't throw but instead coerce the bounds values. So need to check if the slice can be taken beyond the bounds of the allocated buffer, and if writing to that slice can write into the parent buffer. Additional tests have been added to check for some of these cases because they have happened. Despite the tests in question here. Demonstrating it didn't accomplish what it was originally intended to do. Feel free to merge this. Because as they currently stand the reason for how they were implemented is confusing. I'll open another PR with additional tests that I believe were the original intent. |
|
@trevnorris Are you comfortable enough with this to give it an official LGTM? If so, I'd like to merge it. |
|
Go right ahead. LGTM. |
The value of `j` is never used and the value of `offset` is never changed. So the loops executes the same tests multiple times without apparent explanation. (Perhaps it was originally some kind of crude benchmarking?)
|
Jenkins CI for this change: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/838/ |
Remove loops executing the same tests multiple times. PR-URL: nodejs#1874 Reviewed-By: Trevor Norris <trev.norris@gmail.com>
|
Merged in 88d7904 |
The value of
jis never used and the value ofoffsetis never changed. So the loops execute the same tests
multiple times without apparent explanation. (Perhaps it was
originally some kind of crude benchmarking?)