-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
[v6.x backport] doc,assert: document stackStartFunction in fail #14427
Conversation
aac0e9e
to
ec0149a
Compare
|
7b8fcf8
to
f4691bb
Compare
f9419c2
to
403c465
Compare
aaf4e13
to
31f572c
Compare
@refack rebase onto v6.x-master and I'm confused by the PR title "document ...", but it implements, as well as documents? Adding yet another argument to |
Staring slightly harder, this seems to be a couple changes squashed into one commit: both a refactor of lib/assert.js, and the documentation of a pre-existing API? @refack need some clarification here, please. |
ec0149a
to
d60e9b7
Compare
@sam-github it is indeed a combination of multiple commits and if I read the change correct it also changes assert.fail a tiny bit. It also includes e.g. #12293 |
Decided not to move forward |
d60e9b7
to
ded595f
Compare
* refactor the code 1. Rename private functions 2. Use destructuring 3. Remove obsolete comments * remove eslint rule PR-URL: nodejs#13862 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
ded595f
to
eeab2a3
Compare
@refack the lts working group decided to not land this independent of how hard it was to backport. We gate which minor updates we land on the branch. Closing again, feel free to continue discussion though. |
As far as I remember (from listening to the discussion) the decision was based on this PR affecting the output of The only change is improving the default error for no args diff --git a/test/parallel/test-assert-fail.js b/test/parallel/test-assert-fail.js
index a64cfdb3abb..ebd962be751 100644
assert.throws(
() => { assert.fail(); },
- /^AssertionError: undefined undefined undefined$/
+ AssertionError,
+ 'Failed'
); So I'd like this to be reconsidered. |
So the part that we thought was a blocker we've actually already backported? Seems a bit silly not to backport this now then. +1 to backporting. |
Refs: #13862
Was asked to port to
v8.x
but I ported tov6.x
by mistake, so here it is if you want it 🤷♂️/cc @nodejs/lts
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
assert