test: simplify test skipping by vsemozhetbyt · Pull Request #14021 · nodejs/node · GitHub

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

Make common.skip() exit. Also add common.printSkipMessage() for partial skips. Fixes: test: a small common.skip() improvement proposal #14016
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.

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/

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

Some unstable results due to flaky tests.

A few nits

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

should be just 'Win32 does not support signals.'

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

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

Not for this PR.

Ref: #14039

Conflict resolved, comments addressed.

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

Some results are unstable with flaky tests.

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 🤷‍♂️

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

Duly noted.

@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

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/

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

Landed in 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>

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

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

@MylesBorins
Backport: #14838

* 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>

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 mentioned this pull request Jul 1, 2017

test: a small common.skip() improvement proposal #14016

Closed

vsemozhetbyt mentioned this pull request Jul 1, 2017

repl, url, tools: use no-use-before-define ESLint rule #14032

Closed

2 tasks

refack approved these changes Jul 2, 2017

View reviewed changes

refack mentioned this pull request Jul 2, 2017

tools: add eslint rule for hasCrypto checking #13813

Closed

2 tasks

vsemozhetbyt added 2 commits July 4, 2017 04:51

test: make common.skip() exit

ab91eb8

Also add common.printSkipMessage() for partial skips. Fixes: #14016

test: don't make needless things before skip

881c785

vsemozhetbyt closed this Jul 4, 2017

vsemozhetbyt deleted the test-skip branch July 4, 2017 09:45

addaleax mentioned this pull request Jul 11, 2017

v8.2.0 proposal #13744

Merged

vsemozhetbyt mentioned this pull request Jul 14, 2017

http2: Introducing http/2 #14239

Closed

4 tasks

MylesBorins added the backport-requested-v6.x label Aug 15, 2017

vsemozhetbyt mentioned this pull request Aug 15, 2017

[v6.x backport] test: simplify test skipping #14838

Closed

3 tasks

MylesBorins added backported-to-v6.x and removed backport-requested-v6.x labels Aug 16, 2017

MylesBorins mentioned this pull request Aug 16, 2017

v6.11.3-proposal #14852

Merged

test: simplify test skipping by vsemozhetbyt · Pull Request #14021 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: simplify test skipping - #14021

test: simplify test skipping#14021vsemozhetbyt wants to merge 2 commits intonodejs:masternodejs/node:masterfrom vsemozhetbyt:test-skipCopy head branch name to clipboard

Conversation

vsemozhetbyt commented Jul 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

vsemozhetbyt commented Jul 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsemozhetbyt commented Jul 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack Jul 2, 2017

Choose a reason for hiding this comment

Uh oh!

refack Jul 2, 2017

Choose a reason for hiding this comment

Uh oh!

refack Jul 2, 2017

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Jul 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack Jul 2, 2017

Choose a reason for hiding this comment

Uh oh!

refack Jul 2, 2017

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Jul 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Jul 2, 2017

Uh oh!

vsemozhetbyt commented Jul 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsemozhetbyt commented Jul 2, 2017

Uh oh!

vsemozhetbyt commented Jul 4, 2017

Uh oh!

benjamingr commented Jul 4, 2017

Uh oh!

vsemozhetbyt commented Jul 4, 2017

Uh oh!

MylesBorins commented Aug 15, 2017

Uh oh!

vsemozhetbyt commented Aug 15, 2017

Uh oh!

vsemozhetbyt commented Aug 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

test: simplify test skipping#14021
vsemozhetbyt wants to merge 2 commits into
nodejs:masterfrom
vsemozhetbyt:test-skip

vsemozhetbyt commented Jul 1, 2017 •
edited
Loading

vsemozhetbyt commented Jul 1, 2017 •
edited
Loading

vsemozhetbyt commented Jul 1, 2017 •
edited
Loading

vsemozhetbyt Jul 2, 2017 •
edited
Loading

vsemozhetbyt commented Jul 2, 2017 •
edited
Loading

vsemozhetbyt commented Jul 2, 2017 •
edited
Loading