FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v6.x backport] doc,assert: document stackStartFunction in fail by refack · Pull Request #14427 · nodejs/node · GitHub

/ node Public

[v6.x backport] doc,assert: document stackStartFunction in fail - #14427

Closed
refack wants to merge 1 commit into
nodejs:v6.x-stagingfrom
refack:v8-backport-13862
Closed

[v6.x backport] doc,assert: document stackStartFunction in fail#14427
refack wants to merge 1 commit into
nodejs:v6.x-stagingfrom
refack:v8-backport-13862

Conversation

refack commented Jul 22, 2017
edited
Loading

Copy link
Copy Markdown
Contributor

Refs: #13862
Was asked to port to v8.x but I ported to v6.x by mistake, so here it is if you want it 🤷‍♂️

/cc @nodejs/lts

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 added assert Issues and PRs related to the assert subsystem. lts-agenda semver-minor PRs that contain new features and should be released in the next minor version. test Issues and PRs related to the tests. labels Jul 22, 2017
nodejs-github-bot added assert Issues and PRs related to the assert subsystem. tools Issues and PRs related to the tools directory. v6.x labels Jul 22, 2017

refack commented Jul 22, 2017

Copy link
Copy Markdown
Contributor Author

refack commented Jul 22, 2017

Copy link
Copy Markdown
Contributor Author

refack commented Jul 22, 2017

Copy link
Copy Markdown
Contributor Author
not ok 179 parallel/test-net-listen-port-option
  ---
  duration_ms: 60.135
  severity: fail
  stack: |-
    timeout
  ...

Copy link
Copy Markdown
Contributor

@refack rebase onto v6.x-master and not ok 179 parallel/test-net-listen-port-option will dissappear, at least.

I'm confused by the PR title "document ...", but it implements, as well as documents?

Adding yet another argument to assert.fail() seems reasonably safe, any reason we shouldn't backport this?

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Member

@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

refack commented Sep 19, 2017

Copy link
Copy Markdown
Contributor Author

@refack need some clarification here, please.

Original the request was to backport #13862 to v8.x. Out of haste I ported it to v6.x by mistake. To smooth port I dragged in #13974 as well.
It was major in master/v8.x but here AFAICT it's minor.

Anyway if it looks iffy feel free to reject it

Copy link
Copy Markdown
Contributor

Decided not to move forward

refack commented Oct 10, 2017

Copy link
Copy Markdown
Contributor Author

Now that #15479 (the backport of #12293) landed. This has become completely minor.

refack reopened this Oct 10, 2017
refack force-pushed the v8-backport-13862 branch from d60e9b7 to ded595f Compare October 10, 2017 15:30
refack changed the base branch from v6.x-staging to v6.x October 10, 2017 15:30
refack changed the base branch from v6.x to v6.x-staging October 10, 2017 15:30
* 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>
refack force-pushed the v8-backport-13862 branch from ded595f to eeab2a3 Compare October 10, 2017 15:33

refack commented Oct 10, 2017
edited
Loading

Copy link
Copy Markdown
Contributor Author

Copy link
Copy Markdown
Contributor

@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.

refack commented Oct 10, 2017

Copy link
Copy Markdown
Contributor Author

As far as I remember (from listening to the discussion) the decision was based on this PR affecting the output of assert.fail. That part was just backported by #15479.
So now this PR is almost completely documentation and refactoring.

The only change is improving the default error for no args assert.fail():

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.

gibfahn commented Oct 10, 2017

Copy link
Copy Markdown
Member

As far as I remember (from listening to the discussion) the decision was based on this PR affecting the output of assert.fail. That part was just backported by #15479.

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.

refack deleted the v8-backport-13862 branch October 24, 2017 13:53
refack restored the v8-backport-13862 branch October 24, 2017 13:53
refack deleted the v8-backport-13862 branch September 5, 2018 22:30
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

assert Issues and PRs related to the assert subsystem. semver-minor PRs that contain new features and should be released in the next minor version. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants


Back | FazBrowse Home | New Git URL