[v12.x backport] test: refactor common.expectsError - #31449
Conversation
Sorry, something went wrong.
Sorry, something went wrong.
|
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 ...
|
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
|
All comments addressed @BridgeAR if you want to take another look. |
Sorry, something went wrong.
|
This passed although doesn't include my latest (minor) tweak to address review comments |
Sorry, something went wrong.
|
I've rebased against v12.x to fixed conflicts. |
Sorry, something went wrong.
Sorry, something went wrong.
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>
|
rebased |
Sorry, something went wrong.
|
CI: https://ci.nodejs.org/job/node-test-pull-request/30315/ |
Sorry, something went wrong.
Sorry, something went wrong.
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>
|
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. |
Sorry, something went wrong.
|
@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 |
Sorry, something went wrong.
Checklist
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)