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: simplify test skipping #14021

Closed
wants to merge 2 commits into from
Closed

test: simplify test skipping #14021

wants to merge 2 commits into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Jul 1, 2017 •
edited
Loading

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)

test, doc

  1. Make common.skip() exit. Also add common.printSkipMessage() for partial skips. Fixes: test: a small common.skip() improvement proposal #14016

  2. Don't make needless things before skip

PR is big but seems easy to skim. Maybe it would be more convenient to review it commit by commit.

@nodejs-github-bot nodejs-github-bot added addons Issues and PRs related to native addons. async_hooks Issues and PRs related to the async hooks subsystem. doc Issues and PRs related to the documentations. dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Jul 1, 2017
@vsemozhetbyt vsemozhetbyt removed addons Issues and PRs related to native addons. async_hooks Issues and PRs related to the async hooks subsystem. inspector Issues and PRs related to the V8 inspector protocol tools Issues and PRs related to the tools directory. labels Jul 1, 2017
@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Jul 1, 2017 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/8910/ (failed due to a typo)

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

@vsemozhetbyt vsemozhetbyt mentioned this pull request Jul 1, 2017
Closed
@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Jul 1, 2017 •
edited
Loading

Sorting in the doc fixed. New CI: https://ci.nodejs.org/job/node-test-pull-request/8912/

Some unstable results due to flaky tests.

@vsemozhetbyt vsemozhetbyt mentioned this pull request Jul 1, 2017
Closed
2 tasks
refack
refack approved these changes Jul 2, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

A few nits

test/parallel/test-net-connect-options-ipv6.js Outdated
@@ -81,7 +79,7 @@ function tryConnect() {
if (host)
tryConnect();
else {
common.skip('no IPv6 localhost support');
common.printSkipMessage('no IPv6 localhost support');
Copy link
Contributor

Choose a reason for hiding this comment

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

also I think this is a skip (exiting will close the server, and also you can flip the order, server.close then skip`

test/parallel/test-process-remove-all-signal-listeners.js Outdated
@@ -7,7 +7,6 @@ const spawn = require('child_process').spawn;
if (common.isWindows) {
common.skip('Win32 doesn\'t have signals, just a kind of ' +
'emulation, insufficient for this test to apply.');
Copy link
Contributor

Choose a reason for hiding this comment

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

should be just 'Win32 does not support signals.'

test/parallel/test-setproctitle.js
return common.skip('Windows does not have "ps" utility');
}
if (common.isWindows)
common.skip('Windows does not have "ps" utility');
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not true. I'll open an issue to fix this test.

Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt Jul 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Are any actions from me required in this case for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ref: #14039

@refack refack mentioned this pull request Jul 2, 2017
Closed
2 tasks
@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Jul 2, 2017 •
edited
Loading

Conflict resolved, comments addressed.

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

Some results are unstable with flaky tests.

@refack
Copy link
Contributor

refack commented Jul 2, 2017

Conflict resolved, comments addressed.

For my sanity, I'd say in such huge PRs address the comments in a new commit (rebasing aside). Now I need to trust you or go over everything again 🤷‍♂️

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Jul 2, 2017 •
edited
Loading

Sorry. In addition to two files mentioned in your comments, the only parallel/test-crypto-pbkdf2.js was touched due to 48660fb

Duly noted.

@vsemozhetbyt
Copy link
Contributor Author

@nodejs/collaborators, this PR changes a heavily used method from common library (and adds an associated one), so I mention all collaborators to be sure the news is spread and no objections exist. (Refs once more: #14016)

@vsemozhetbyt
test: make common.skip() exit
ab91eb8 
Also add common.printSkipMessage() for partial skips.

Fixes: #14016
@vsemozhetbyt
test: don't make needless things before skip
881c785
@vsemozhetbyt
Copy link
Contributor Author

Conflict resolved in (due to cc1a47d):

test/parallel/test-tls-alert.js
test/parallel/test-tls-alert-handling.js
test/parallel/test-tls-client-verify.js
test/parallel/test-tls-npn-server-client.js
test/parallel/test-tls-server-verify.js
test/parallel/test-tls-sni-option.js
test/parallel/test-tls-sni-server-client.js
test/parallel/test-tls-startcom-wosign-whitelist.js

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

@benjamingr
Copy link
Member

I'm +0 on those changes, the benefit is marginal IMO and it adds churn but the end code is slightly cleaner.

@vsemozhetbyt
Copy link
Contributor Author

Landed in 2d2986a

@vsemozhetbyt vsemozhetbyt deleted the test-skip branch July 4, 2017 09:45
vsemozhetbyt added a commit that referenced this pull request Jul 4, 2017
@vsemozhetbyt
test: simplify test skipping
2d2986a 
* Make common.skip() exit.

  Also add common.printSkipMessage() for partial skips.

* Don't make needless things before skip

PR-URL: #14021
Fixes: #14016
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@vsemozhetbyt @addaleax
test: simplify test skipping
ccf69da 
* Make common.skip() exit.

  Also add common.printSkipMessage() for partial skips.

* Don't make needless things before skip

PR-URL: #14021
Fixes: #14016
Reviewed-By: Refael Ackermann <refack@gmail.com>
@addaleax addaleax mentioned this pull request Jul 11, 2017
Merged
@vsemozhetbyt vsemozhetbyt mentioned this pull request Jul 14, 2017
Closed
4 tasks
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@vsemozhetbyt @addaleax
test: simplify test skipping
32a8f36 
* Make common.skip() exit.

  Also add common.printSkipMessage() for partial skips.

* Don't make needless things before skip

PR-URL: #14021
Fixes: #14016
Reviewed-By: Refael Ackermann <refack@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@vsemozhetbyt @Fishrock123
test: simplify test skipping
bc99efc 
* Make common.skip() exit.

  Also add common.printSkipMessage() for partial skips.

* Don't make needless things before skip

PR-URL: #14021
Fixes: #14016
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins
Copy link
Contributor

@vsemozhetbyt would you be able to backport this to v6.x?

I totally missed this coming through or would have likely blocked on such a large churn going against the repo. With 330 files changed in this single commit we have a fairly huge delta now when backporting any future test refactors / changes.

@Trott has been working on similar backports but submitted each subsystem or group of specific tests per PR, which makes it much easier to backport.

In future if we are going to do such large churn can you please coordinate with @nodejs/lts and potentially get a backport ready in advance?

@vsemozhetbyt
Copy link
Contributor Author

@MylesBorins I shall try to backport and coordinate with @nodejs/lts for such PRs if there are any (I am personally a bit burdened already with my gloomy reputation as "king of churn").

@vsemozhetbyt vsemozhetbyt mentioned this pull request Aug 15, 2017
Closed
3 tasks
@vsemozhetbyt
Copy link
Contributor Author

@MylesBorins
Backport: #14838

MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@vsemozhetbyt @MylesBorins
test: simplify test skipping
44d3047 
* Make common.skip() exit.

  Also add common.printSkipMessage() for partial skips.

* Don't make needless things before skip

Backport-PR-URL: #14838
PR-URL: #14021
Fixes: #14016
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
@vsemozhetbyt @MylesBorins
test: simplify test skipping
1633f8b 
* Make common.skip() exit.

  Also add common.printSkipMessage() for partial skips.

* Don't make needless things before skip

Backport-PR-URL: #14838
PR-URL: #14021
Fixes: #14016
Reviewed-By: Refael Ackermann <refack@gmail.com>
This was referenced Sep 5, 2017
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vsemozhetbyt @refack @benjamingr @MylesBorins @nodejs-github-bot