-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: http2 stored settings returned when present #15751
Conversation
This is just a nit but the current implementation requires checking the code coverage to confirm that the test is succeeding (the test will never fail, even if the caching is removed). A simple change could be to store the local settings and remote settings that are returned from the first call and then confirming that the objects that are retrieved the 2nd time are one and the same ( const localSettings = stream.session.localSettings;
const remoteSettings = stream.session.remoteSettings;
assertSettings(localSettings);
assertSettings(remoteSettings);
// Test that stored settings are returned when called for second time
assert.strictEqual(stream.session.localSettings, localSettings);
assert.strictEqual(stream.session.remoteSettings, remoteSettings); |
1582da4
to
0c6f788
Compare
@apapirovski Thanks for the comment 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Landed as fabd618 |
Refs: #14985 PR-URL: nodejs/node#15751 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This PR adds test to confirm that stored settings are returned when they're available in Http2Stream
It covers following lines:
node/lib/internal/http2/core.js
Lines 805 to 806 in 2f8ddb2
node/lib/internal/http2/core.js
Lines 820 to 821 in 2f8ddb2
Refs: #14985
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
test, http2