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

test: abstract skip functionality to common by Fishrock123 · Pull Request #6697 · nodejs/node · GitHub

/ node Public

test: abstract skip functionality to common - #6697

Merged
Fishrock123 merged 1 commit into
nodejs:masterfrom
Fishrock123:common-skip
May 12, 2016
ConversationCommits (1)ChecksFiles changed
Merged

test: abstract skip functionality to common#6697
Fishrock123 merged 1 commit into
nodejs:masterfrom
Fishrock123:common-skip

Conversation

Fishrock123 commented May 11, 2016

Copy link
Copy Markdown
Contributor
Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

The tap skipping output is so prevalent yet obscure in nature that we ought to move it into it's own function in test/common.js

cc @nodejs/testing

nodejs-github-bot added the test Issues and PRs related to the tests. label May 11, 2016
Fishrock123 reviewed May 11, 2016
View reviewed changes
Comment thread test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-ascii.js Outdated

Fishrock123 May 11, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Oops, still need to fix these!

Fishrock123 commented May 11, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Here's the actual function in test/common.js: https://github.com/nodejs/node/pull/6697/files#diff-8736c5cbff21e1dee18b0c86d3d2689dR468

Fishrock123 commented May 11, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/2592/ (Green)

Trott commented May 11, 2016

Copy link
Copy Markdown
Member

LGTM if CI has no complaints

Fishrock123 mentioned this pull request May 11, 2016
Closed

santigimeno commented May 11, 2016

Copy link
Copy Markdown
Member

LGTM

Trott commented May 11, 2016

Copy link
Copy Markdown
Member

Nit: It might be good to name the function more precisely, as it does not actually skip. It outputs the TAP-formatted skip message. I would expect skip() to exit the process too. So either add process.exit() to skip() or else change the function to something like printSkipTapOutput() (although that's not exactly aesthetically pleasing).

Totally a nit, feel free to ignore, I know process.exit() causes some trepidation in some situations, and I'm fine with this as it is.

Fishrock123 commented May 11, 2016

Copy link
Copy Markdown
Contributor Author

Maybe just printSkip(msg)? Idk

Fishrock123 commented May 11, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

The reason I went with this is iirc because it causes a skip even if the test fails afterwards?

Trott commented May 11, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

The more I think about it, I wouldn't expect a common.whatever() call to exit my process anyway, so it's probably a bad suggestion.

orangemocha mentioned this pull request May 12, 2016
Closed
4 tasks

indutny commented May 12, 2016

Copy link
Copy Markdown
Member

LGTM

test: abstract skip functionality to common
52bae22 
The tap skipping output is so prevalent yet obscure in nature that we
ought to move it into it's own function in test/common.js

PR-URL: nodejs#6697
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Fishrock123 merged commit 52bae22 into nodejs:master May 12, 2016
evanlucas pushed a commit that referenced this pull request May 17, 2016
test: abstract skip functionality to common
b1f58ed 
The tap skipping output is so prevalent yet obscure in nature that we
ought to move it into it's own function in test/common.js

PR-URL: #6697
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
evanlucas mentioned this pull request May 17, 2016
Merged

MylesBorins commented Jun 2, 2016

Copy link
Copy Markdown
Contributor

@Fishrock123 I'm going to put this in dont-land-in-lts but please feel free to do a manual backport

Fishrock123 commented Jun 2, 2016

Copy link
Copy Markdown
Contributor Author

Huh. Backporting doesn't seem to work at all. 10 obscure errors.

I'll just redo the patch for v4.x, it shoudn't be difficult, and this will likely cause backport conflicts if we don't.

MylesBorins commented Jun 2, 2016

Copy link
Copy Markdown
Contributor

@Fishrock123 that's what I figured would happen. Totally up for the manual backport thanks 👍

Fishrock123 mentioned this pull request Jun 2, 2016
Closed
3 tasks

Fishrock123 commented Jun 2, 2016

Copy link
Copy Markdown
Contributor Author

backport @ #7114

MylesBorins pushed a commit that referenced this pull request Jun 2, 2016
test: abstract skip functionality to common
964ee06 
The tap skipping output is so prevalent yet obscure in nature that we
ought to move it into it's own function in test/common.js

Re-do of 52bae22 for v4.x

Ref: #6697
PR-URL: #7114
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 2, 2016
test: abstract skip functionality to common
b6e3c46 
The tap skipping output is so prevalent yet obscure in nature that we
ought to move it into it's own function in test/common.js

Re-do of 52bae22 for v4.x

Ref: #6697
PR-URL: #7114
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
test: abstract skip functionality to common
4067cde 
The tap skipping output is so prevalent yet obscure in nature that we
ought to move it into it's own function in test/common.js

Re-do of 52bae22 for v4.x

Ref: #6697
PR-URL: #7114
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
test: abstract skip functionality to common
62bfe18 
The tap skipping output is so prevalent yet obscure in nature that we
ought to move it into it's own function in test/common.js

Re-do of 52bae22 for v4.x

Ref: #6697
PR-URL: #7114
Reviewed-By: Myles Borins <myles.borins@gmail.com>
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL