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

[v12.x backport] test: refactor common.expectsError #31449

Closed
wants to merge 1 commit into from

Conversation

sxa
Copy link
Member

@sxa sxa commented Jan 21, 2020

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

Ever had one of those moments when you wish you hadn't started a backport but wanted to see it through to completion? I'm glad this is done now and it'll definitely make subsequent backports easer (would have helped for the other one I did this evening). Understandable there are quite a few adjustments in this due to differences in test cases between v12.x and master which meant that many fixups were required, but this now passes the tests.

I've only verified that this is ok on xLinux.

If someone can kick off the test-commit job that would be appreciated.

For reviewers, it's probably fairly obvious but the "fixup" commit in here contains most of the changes I had to make manually (plus a couple of typo fixes from my first merge resolution)

@nodejs-github-bot nodejs-github-bot added addons Issues and PRs related to native addons. async_hooks Issues and PRs related to the async hooks subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. test Issues and PRs related to the tests. v12.x labels Jan 21, 2020
@sxa sxa requested review from BridgeAR and targos January 21, 2020 22:06
@nodejs-github-bot
Copy link
Collaborator

@sxa
Copy link
Member Author

sxa commented Jan 21, 2020

https://ci.nodejs.org/job/node-test-commit-linux/nodes=centos7-64-gcc6/32389/consoleFull (Thanks @BethGriggs for kicking it off) failed the PR test job due to the failure shown below with test-http2-reset-fllood. Since this PR only changes tests it seems likely that this is a machine specific issue, or I was just unlucky on an intermittent test (It's known flakey on FreeBSD as per #29802 - perhaps #30534 will assist with this - although if it's already been backported - it's called out in #31368 - then perhaps that hasn't fixed it!). Other platforms/Linux distributions that have completed as I write this have not failed in the same way as per https://ci.nodejs.org/job/node-test-commit/35039/console

22:28:15 not ok 1169 parallel/test-http2-reset-flood
22:28:15   ---
22:28:15   duration_ms: 0.328
22:28:15   severity: fail
22:28:15   exitcode: 1
22:28:15   stack: |-
22:28:15     events.js:298
22:28:15           throw er; // Unhandled 'error' event
22:28:15           ^
22:28:15     
22:28:15     Error: This socket has been ended by the other party
22:28:15         at Socket.writeAfterFIN [as write] (net.js:451:14)
22:28:15         at Immediate.writeRequests (/home/iojs/build/workspace/node-test-commit-linux/nodes/centos7-64-gcc6/test/parallel/test-http2-reset-flood.js:72:12)
22:28:15         at processImmediate (internal/timers.js:456:21)
22:28:15     Emitted 'error' event on Socket instance at:
22:28:15         at emitErrorNT (net.js:1336:8)
22:28:15         at processTicksAndRejections (internal/process/task_queues.js:84:21) {
22:28:15       code: 'EPIPE'
22:28:15     }
22:28:15   ...

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

test/parallel/test-fs-whatwg-url.js Outdated Show resolved Hide resolved
test/parallel/test-crypto.js Outdated Show resolved Hide resolved
@sxa sxa force-pushed the v12.x-31092 branch 5 times, most recently from 44a6565 to 9f59f39 Compare January 22, 2020 13:44
@sxa
Copy link
Member Author

sxa commented Jan 22, 2020

All comments addressed @BridgeAR if you want to take another look.

@sxa
Copy link
Member Author

sxa commented Jan 22, 2020

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

This passed although doesn't include my latest (minor) tweak to address review comments

@MylesBorins
Copy link
Contributor

I've rebased against v12.x to fixed conflicts.

@nodejs-github-bot
Copy link
Collaborator

@guybedford guybedford removed the esm Issues and PRs related to the ECMAScript Modules implementation. label Mar 12, 2020
@codebytere codebytere force-pushed the v12.x-staging branch 2 times, most recently from 63a03d2 to d577190 Compare March 31, 2020 23:57
This completely refactors the `expectsError` behavior: so far it's
almost identical to `assert.throws(fn, object)` in case it was used
with a function as first argument. It had a magical property check
that allowed to verify a functions `type` in case `type` was passed
used in the validation object. This pattern is now completely removed
and `assert.throws()` should be used instead.

The main intent for `common.expectsError()` is to verify error cases
for callback based APIs. This is now more flexible by accepting all
validation possibilites that `assert.throws()` accepts as well. No
magical properties exist anymore. This reduces surprising behavior
for developers who are not used to the Node.js core code base.

This has the side effect that `common` is used significantly less
frequent.

Backport-PR-URL: nodejs#31449
PR-URL: nodejs#31092
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@MylesBorins
Copy link
Contributor

rebased

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 1, 2020

@MylesBorins
Copy link
Contributor

BSD failure might be known flake

#29802
#32595

MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
This completely refactors the `expectsError` behavior: so far it's
almost identical to `assert.throws(fn, object)` in case it was used
with a function as first argument. It had a magical property check
that allowed to verify a functions `type` in case `type` was passed
used in the validation object. This pattern is now completely removed
and `assert.throws()` should be used instead.

The main intent for `common.expectsError()` is to verify error cases
for callback based APIs. This is now more flexible by accepting all
validation possibilites that `assert.throws()` accepts as well. No
magical properties exist anymore. This reduces surprising behavior
for developers who are not used to the Node.js core code base.

This has the side effect that `common` is used significantly less
frequent.

Backport-PR-URL: #31449
PR-URL: #31092
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@MylesBorins
Copy link
Contributor

landed in d568efc

@MylesBorins MylesBorins closed this Apr 1, 2020
@sxa
Copy link
Member Author

sxa commented Apr 1, 2020

Thanks @MylesBorins - I hadn't got back to doing the rebase after the release went out as it looked rather non-trivial to redo with the modified history on this one, but should be a good one to get in.

@MylesBorins
Copy link
Contributor

@sxa555 it is landed and in!

FWIW I made a new branch off of staging and cherry-picked your initial commit (rather than rebasing). There was only a single conflict, so it was rather straight forward all things considered

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons Issues and PRs related to native addons. async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants