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: fix assert.throws in test-http-invalid-urls #15678

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 29, 2017

When the second argument to assert.throws() is a string, it is not
treated as the expected error message but rather the message that the
assertion should display if no error is thrown. Ths change fixes that
error in test-http-invalid-urls.js.

Instead of skipping the test when there is no crypto, the test is now
run but with http only. https is skipped.

Logging was fixed. Previously, errors would be written out as being in
the [object Object] module rather than http or https.

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

test

@Trott Trott added the test Issues and PRs related to the tests. label Sep 29, 2017
@Trott
Copy link
Member Author

Trott commented Sep 29, 2017

@BridgeAR

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Sep 29, 2017

function test(host) {
['get', 'request'].forEach((method) => {
[http, https].forEach((module) => {
assert.throws(() => module[method](host, () => {
common.expectsError(() => module[method](host, () => {
throw new Error(`${module}.${method} should not connect to ${host}`);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: how about replacing this with common.mustNotCall()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. And while I'm at it, I can add { and } around the arrow function body because there's no need for it to implicitly return a value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Found other problems in the process. Refactored/fixed/commented in the commit message.

When the second argument to `assert.throws()` is a string, it is not
treated as the expected error message but rather the message that the
assertion should display if no error is thrown. Ths change fixes that
error in `test-http-invalid-urls.js`.

Instead of skipping the test when there is no crypto, the test is now
run but with `http` only. `https` is skipped.

Logging was fixed. Previously, errors would be written out as being in
the `[object Object]` module rather than `http` or `https`.
@Trott
Copy link
Member Author

Trott commented Sep 29, 2017

@BridgeAR
Copy link
Member

BridgeAR commented Oct 1, 2017

Landed in 2e215f1

@BridgeAR BridgeAR closed this Oct 1, 2017
BridgeAR pushed a commit that referenced this pull request Oct 1, 2017
When the second argument to `assert.throws()` is a string, it is not
treated as the expected error message but rather the message that the
assertion should display if no error is thrown. Ths change fixes that
error in `test-http-invalid-urls.js`.

Instead of skipping the test when there is no crypto, the test is now
run but with `http` only. `https` is skipped.

Logging was fixed. Previously, errors would be written out as being in
the `[object Object]` module rather than `http` or `https`.

PR-URL: #15678
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins
Copy link
Contributor

When cherry-picked to 8.x this test is failing. Could you please backport

@Trott
Copy link
Member Author

Trott commented Oct 4, 2017

@MylesBorins The test change is dependent on #14423 which is semver-major. I'll label this "dont-land" on 8, 6, and 4.

addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 4, 2017
When the second argument to `assert.throws()` is a string, it is not
treated as the expected error message but rather the message that the
assertion should display if no error is thrown. Ths change fixes that
error in `test-http-invalid-urls.js`.

Instead of skipping the test when there is no crypto, the test is now
run but with `http` only. `https` is skipped.

Logging was fixed. Previously, errors would be written out as being in
the `[object Object]` module rather than `http` or `https`.

PR-URL: nodejs/node#15678
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@Trott Trott deleted the assert-throws-booboo branch January 13, 2022 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants