Skip to content
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: refactor test-http-expect-continue #19625

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 27, 2018

Use common.mustCall() where appropriate. Remove some logic that is not
required when common.mustCall() is used (incrementor/decrementor to make
sure everything is called the same number of times).

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Use common.mustCall() where appropriate. Remove some logic that is not
required when common.mustCall() is used (incrementor/decrementor to make
sure everything is called the same number of times).
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 27, 2018
@Trott
Copy link
Member Author

Trott commented Mar 27, 2018

console.error('Got full response.');
assert.strictEqual(body, test_res_body, 'Response body doesn\'t match.');
assert.strictEqual(body, test_res_body);
Copy link
Member

Choose a reason for hiding this comment

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

@Trott - is it that the removed error message is too awkward to be of any use, or is there a consensus / guideline to remove error messages from assertion statements?

Copy link
Member Author

@Trott Trott Mar 27, 2018

Choose a reason for hiding this comment

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

By including that assertion message, we suppress the printing of the contents of body and test_res_body when there is in AssertionError. Those values would seem to be useful for debugging. In contrast, the supplied message doesn't provide any useful information IMO.

An alternative would be to replace it with a template literal: `Response body doesn't match. Expected ${test_res_body} but received ${body}.` I prefer simply removing it, but I could go with that if it's deemed important to keep.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the clarification!


const server = http.createServer(handler);
server.on('checkContinue', function(req, res) {
const server = http.createServer();
Copy link
Member

Choose a reason for hiding this comment

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

handler is no longer registered as a 'request' listener?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, given the description in the API docs for checkContinue it probably makes more sense to register a common.mustNotCall request listener.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 28, 2018
@Trott
Copy link
Member Author

Trott commented Mar 29, 2018

Landed in ffe3f9a

@Trott Trott closed this Mar 29, 2018
Trott added a commit to Trott/io.js that referenced this pull request Mar 29, 2018
Use common.mustCall() where appropriate. Remove some logic that is not
required when common.mustCall() is used (incrementor/decrementor to make
sure everything is called the same number of times).

PR-URL: nodejs#19625
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Apr 2, 2018
Use common.mustCall() where appropriate. Remove some logic that is not
required when common.mustCall() is used (incrementor/decrementor to make
sure everything is called the same number of times).

PR-URL: #19625
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Apr 4, 2018
BethGriggs pushed a commit that referenced this pull request Dec 4, 2018
Use common.mustCall() where appropriate. Remove some logic that is not
required when common.mustCall() is used (incrementor/decrementor to make
sure everything is called the same number of times).

PR-URL: #19625
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 4, 2018
@Trott Trott deleted the refactor-expect-continue branch January 13, 2022 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants