-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
[v8.x] test: bump test/common to master #14459
Conversation
PR-URL: nodejs#14011 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
``` git checkout master test/common && \ git commit -m 'test: bump test/common to master' ```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great.
@@ -23,7 +23,7 @@ assert.throws( | |||
code: 'ERR_ASSERTION', | |||
type: assert.AssertionError, | |||
operator: undefined, | |||
actual: undefined, | |||
actual: 'custom message', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this semver-major
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not accompanied by any changes to lib
or src
, so it really can’t be. I have to admit I didn’t really figure out why this is required, if you like, feel free to do the digging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember now. Before this change expectsError
only checked ['message', 'type','code']
now it compares name
if provided. And for AssertionError
also ['generatedMessage', 'actual', 'expected', 'operator']
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so it sounds like everything is working as expected and better than before? :)
@@ -166,7 +166,7 @@ assert.throws(() => { | |||
}, common.expectsError({ code: 'TEST_ERROR_1', type: RangeError })); | |||
}, common.expectsError({ | |||
code: 'ERR_ASSERTION', | |||
message: /^.+ is not the expected type \S/ | |||
message: /^.+ is not instance of \S/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not semver-major
since it's a change in expectsError
Backport-PR-URL: nodejs#14459 Backport-Reviewed-By: Refael Ackermann <[email protected]> PR-URL: nodejs#14011 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
``` git checkout master test/common && \ git commit -m 'test: bump test/common to master' ``` PR-URL: nodejs#14459 Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: nodejs#14459 Reviewed-By: Refael Ackermann <[email protected]>
should this be backported? |
ping @addaleax |
No, I don’t think this really can be backported. If we want, we can bump |
Backport one commit from #14011, perform a full copy of
test/common
to what’s inmaster
and fix up tests accordingly (with the solutions that were used onmaster
, too).This is not pretty, but right now it seems like the most feasible solution to avoid future merge conflicts.
/cc @refack @BridgeAR