FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: http2 stored settings returned when present by trivikr · Pull Request #15751 · nodejs/node · GitHub

/ node Public

test: http2 stored settings returned when present - #15751

Closed
trivikr wants to merge 1 commit into
nodejs:masterfrom
trivikr:test-http2-session-settings
ConversationCommits (1)ChecksFiles changed
Closed

test: http2 stored settings returned when present#15751
trivikr wants to merge 1 commit into
nodejs:masterfrom
trivikr:test-http2-session-settings

Conversation

trivikr commented Oct 3, 2017

Copy link
Copy Markdown
Member

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

if (settings !== undefined)
return settings;

node/lib/internal/http2/core.js

Lines 820 to 821 in 2f8ddb2

if (settings !== undefined)
return settings;

Refs: #14985

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test, http2

nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 3, 2017

lpinca commented Oct 3, 2017

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/10392/

mscdex added the http2 Issues or PRs related to the http2 subsystem. label Oct 3, 2017

apapirovski commented Oct 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 (strictEqual).

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);

trivikr force-pushed the test-http2-session-settings branch from 1582da4 to 0c6f788 Compare October 3, 2017 15:54

trivikr commented Oct 3, 2017

Copy link
Copy Markdown
Member Author

@apapirovski Thanks for the comment 👍
I've incorporated it in the commit.

benjamingr commented Oct 5, 2017

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/10415/console

mcollina approved these changes Oct 7, 2017
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

mcollina commented Oct 7, 2017

Copy link
Copy Markdown
Member

Landed as fabd618

mcollina closed this Oct 7, 2017
mcollina pushed a commit that referenced this pull request Oct 7, 2017
test: http2 stored settings returned when present
fabd618 
Refs: #14985
PR-URL: #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>
trivikr deleted the test-http2-session-settings branch October 7, 2017 18:35
MylesBorins pushed a commit that referenced this pull request Oct 7, 2017
test: http2 stored settings returned when present
9d2cf15 
Refs: #14985
PR-URL: #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>
MylesBorins mentioned this pull request Oct 7, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Oct 11, 2017
test: http2 stored settings returned when present
bd49ada 
Refs: #14985
PR-URL: #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>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 12, 2017
test: http2 stored settings returned when present
22ac464 
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 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
jasnell jasnell approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes

Assignees

No one assigned

Labels

http2 Issues or PRs related to the http2 subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

Back | FazBrowse Home | New Git URL