test: increase test-crypto.js strictness #10784
Conversation
Trott
added
crypto
| } | ||
|
|
||
| // Assume that we have at least AES-128-CBC. | ||
| assert.notStrictEqual(0, crypto.getCiphers().length); |
There was a problem hiding this comment.
Without this, getCiphers() could return [], and the test would pass, because the empty set is sorted and unique.
There was a problem hiding this comment.
Without this, getCiphers() could return [], and the test would pass, because the empty set is sorted and unique.
The test will fail because the very next line requires that the array includes aes-128-cbc as an element. So it can't be length 0. So the zero-length check seems redundant to me.
Although looking more closely, I suppose it's possible that getCiphers() would return different arrays each time called. (That would be a bug, but that's the point of tests.) So I should probably assign the result to a variable before all the asserts. Maybe even throw in another call to getCiphers() at the end and confirm that the array it returns is identical to the one it returned in the first place.
There was a problem hiding this comment.
That is reasonably compelling, though an extra assert to be explicit about the condition isn't bad. As you wish, I'm OK either way.
There was a problem hiding this comment.
Eh, well, two people asked "why did you take this out?" and it isn't hurting anything, so maybe best to leave it in after all. I'll put it and the others back.
| exports.getCiphers = internalUtil.cachedResult(() => { | ||
| return internalUtil.filterDuplicateStrings(binding.getSSLCiphers(), true); | ||
| }); | ||
| exports.getCiphers = internalUtil.cachedResult( |
There was a problem hiding this comment.
Unrelated change. I don't object to cleaning up lib/'s use of rocket functions, but I think they should be standalone commits, ideally a standalone commit per file that makes every use in the file consistent (this might or might not be the only such usage, I can't tell from the diff, and the commit message doesn't even mention that there is any cleanup occuring) or just to the entire lib directory to clean up a specific usage aspect.
I also don't think commits called test: ... should touch lib, unless the lib change really clearly is something needed to support the test code (an example might be exporting an internal so that it can be directly used in a unit test)
There was a problem hiding this comment.
Yeah, I'll claim a combination of being tired when I did this, multi-tasking too much, and/or just being lazy. The change originally started in tls.js and morphed into fixing up the test. When the morphing was complete, I shouldn't have included this now-unrelated change. I'll remove it.
| assert.deepStrictEqual(list, sorted); | ||
| // list is sorted and contains no duplicates | ||
| assert(list.every( | ||
| (val, index, array) => index === array.length - 1 || val < array[index + 1] |
There was a problem hiding this comment.
While it works as commented, this is a pretty obfuscated way of doing both a sorted test and a uniqueness test in a single statement. I prefer the approach in
node/test/parallel/test-process-getgroups.js
Lines 30 to 32 in 20efbff
unique() function should not also be sorting. Maybe unique() should be in test/common.js?
There was a problem hiding this comment.
I'll rework it to try to make it a bit more clear what's going on.
| } | ||
|
|
||
| // Assume that we have at least AES-128-CBC. | ||
| assert.notStrictEqual(0, crypto.getCiphers().length); |
There was a problem hiding this comment.
Without this, getCiphers() could return [], and the test would pass, because the empty set is sorted and unique.
| } | ||
|
|
||
| // Assume that we have at least AES-128-CBC. | ||
| assert.notStrictEqual(0, crypto.getCiphers().length); |
There was a problem hiding this comment.
That is reasonably compelling, though an extra assert to be explicit about the condition isn't bad. As you wish, I'm OK either way.
Confirm that `getCiphers()` contains no duplicates.
|
@sam-github I think I've addressed your comments. PTAL. I'll want to expand this test a bit more as I think I've discovered an unexpected behavior, but one thing at a time... |
|
Landed e21126d |
Confirm that `getCiphers()` contains no duplicates. PR-URL: #10784 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Confirm that `getCiphers()` contains no duplicates. PR-URL: nodejs#10784 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Confirm that `getCiphers()` contains no duplicates. PR-URL: nodejs#10784 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Confirm that `getCiphers()` contains no duplicates. PR-URL: nodejs#10784 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Confirm that `getCiphers()` contains no duplicates. PR-URL: nodejs#10784 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Confirm that `getCiphers()` contains no duplicates. PR-URL: #10784 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Confirm that `getCiphers()` contains no duplicates. PR-URL: #10784 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Confirm that
getCiphers()contains no duplicates.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test crypto tls