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.

Sign up for GitHub

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

Jump to bottom

[v8.x backport] assert: make assert.fail doc compliant #14428

Closed
wants to merge 2 commits into from
Closed

[v8.x backport] assert: make assert.fail doc compliant #14428

wants to merge 2 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Jul 22, 2017 •
edited
Loading

Refs: #13862

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

assert

@refack refack added assert Issues and PRs related to the assert subsystem. v8.x labels Jul 22, 2017
@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. v8.x labels Jul 22, 2017
@refack refack mentioned this pull request Jul 22, 2017
Merged
4 tasks
@refack
Copy link
Contributor Author

refack commented Jul 22, 2017

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

@refack
Copy link
Contributor Author

refack commented Jul 22, 2017 •
edited
Loading

@addaleax
Copy link
Member

This does change an existing test so I’d like to run CITGM first:

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/922/

@refack
Copy link
Contributor Author

refack commented Jul 23, 2017 •
edited
Loading

This does change an existing test so I’d like to run CITGM first:

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/922/

CitGM is clean.
(also as you probably noticed, test changes are only additions. The PR started as more extensive, but what landed IMHO is semver-patch) fc46363 (#13974) leaked into this.

addaleax
addaleax reviewed Jul 23, 2017
View reviewed changes
test/parallel/test-assert-fail.js Outdated
assert.throws(
() => { assert.fail(); },
common.expectsError({
code: 'ERR_ASSERTION',
type: assert.AssertionError,
message: 'undefined undefined undefined'
message: 'Failed',
Copy link
Member

Choose a reason for hiding this comment

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

@refack This is not additive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmmm.... right.
This is #13974 leaking into my backport. I can take it out.

Copy link
Member

Choose a reason for hiding this comment

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

I would do that, the alternative is re-discussing the semverness of #13974

@refack
Copy link
Contributor Author

refack commented Jul 23, 2017

Reverted fc46363 (#13974) that leaked into this.
But also reopened discussion on semverity of #13974

@refack
Copy link
Contributor Author

refack commented Jul 23, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/9305/
(Yay for "Rebuild" nodejs/build#672)

@refack
Copy link
Contributor Author

refack commented Jul 23, 2017

@refack
Copy link
Contributor Author

refack commented Jul 24, 2017

Rebased: https://ci.nodejs.org/job/node-test-pull-request/9324/

BridgeAR added 2 commits July 24, 2017 11:56
@BridgeAR @refack
doc,assert: document stackStartFunction in fail
94fecdf 
PR-URL: nodejs#13862
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@BridgeAR @refack
assert: refactor the code
b95ba5b 
1. Rename private functions
2. Use destructuring
3. Remove obsolete comments

PR-URL: nodejs#13862
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@refack
Copy link
Contributor Author

refack commented Jul 24, 2017

Better rebase: https://ci.nodejs.org/job/node-test-commit/11341/

@addaleax
Copy link
Member

LGTM, thank you!

Landed in 78f0c2a, 26785a2 (with the test changes moved back to the commit that they were in originally as chatted about in IRC)

addaleax pushed a commit that referenced this pull request Jul 24, 2017
@BridgeAR @addaleax
doc,assert: document stackStartFunction in fail
78f0c2a 
PR-URL: #13862
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>

Backport-PR-URL: #14428
Backport-Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 24, 2017
@BridgeAR @addaleax
assert: refactor the code
26785a2 
1. Rename private functions
2. Use destructuring
3. Remove obsolete comments

PR-URL: #13862
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>

Backport-PR-URL: #14428
Backport-Reviewed-By: Anna Henningsen <[email protected]>
@addaleax addaleax closed this Jul 24, 2017
@refack refack deleted the v8-port-13862 branch September 18, 2017 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax left review comments

Assignees
No one assigned
Labels
assert Issues and PRs related to the assert subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@refack @addaleax @nodejs-github-bot @BridgeAR