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

test: abstract skip functionality to common #6697

Merged
merged 1 commit into from May 12, 2016
Merged

test: abstract skip functionality to common #6697

merged 1 commit into from May 12, 2016

Conversation

Fishrock123
Copy link
Member

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 nodejs-github-bot added the test Issues and PRs related to the tests. label May 11, 2016
Fishrock123
Fishrock123 reviewed May 11, 2016
View reviewed changes
test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-ascii.js
@@ -7,7 +7,7 @@ const assert = require('assert');
const skipMessage =
'1..0 # Skipped: intensive toString tests due to memory confinements';
Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, still need to fix these!

@Fishrock123
Copy link
Member Author

Fishrock123 commented May 11, 2016 •
edited

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

@Fishrock123
Copy link
Member Author

Fishrock123 commented May 11, 2016 •
edited

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

@Trott
Copy link
Member

Trott commented May 11, 2016

LGTM if CI has no complaints

@Fishrock123 Fishrock123 mentioned this pull request May 11, 2016
Closed
@santigimeno
Copy link
Member

LGTM

@Trott
Copy link
Member

Trott commented May 11, 2016

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
Copy link
Member Author

Maybe just printSkip(msg)? Idk

@Fishrock123
Copy link
Member Author

Fishrock123 commented May 11, 2016 •
edited

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

@Trott
Copy link
Member

Trott commented May 11, 2016 •
edited

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 orangemocha mentioned this pull request May 12, 2016
Closed
4 tasks
@indutny
Copy link
Member

indutny commented May 12, 2016

LGTM

@Fishrock123
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 Fishrock123 merged commit 52bae22 into nodejs:master May 12, 2016
evanlucas pushed a commit that referenced this pull request May 17, 2016
@Fishrock123 @evanlucas
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 evanlucas mentioned this pull request May 17, 2016
Merged
@MylesBorins
Copy link
Member

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

@Fishrock123
Copy link
Member 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
Copy link
Member

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

@Fishrock123 Fishrock123 mentioned this pull request Jun 2, 2016
Closed
3 tasks
@Fishrock123
Copy link
Member Author

backport @ #7114

MylesBorins pushed a commit that referenced this pull request Jun 2, 2016
@Fishrock123
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
@Fishrock123
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
@Fishrock123
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
@Fishrock123
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>
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.

None yet
6 participants
@Fishrock123 @Trott @santigimeno @indutny @MylesBorins @nodejs-github-bot