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.

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

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

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

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Jul 1, 2017

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

Some unstable results due to flaky tests.

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

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

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

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

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

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Jul 2, 2017

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

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)

Also add common.printSkipMessage() for partial skips.

Fixes: #14016
@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
* 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 <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
* 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 <[email protected]>
@addaleax addaleax mentioned this pull request Jul 11, 2017
@vsemozhetbyt vsemozhetbyt mentioned this pull request Jul 14, 2017
4 tasks
addaleax pushed a commit that referenced this pull request Jul 18, 2017
* 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 <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
* 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 <[email protected]>
@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
Copy link
Contributor Author

@MylesBorins
Backport: #14838

MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
* 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 <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
* 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants