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: move benchmark tests out of main test suite #24265

Closed
wants to merge 5 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 9, 2018

Move benchmark tests (which are slow) out of the main test suite. We can
hopefully add them to node-daily-master so that they are still run daily
on CI.

(If someone on Windows could test vcbuild test-benchmark, that would be great.)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Nov 9, 2018
@Trott Trott added test Issues and PRs related to the tests. benchmark Issues and PRs related to the benchmark subsystem. labels Nov 9, 2018
@Trott
Copy link
Member Author

Trott commented Nov 9, 2018

CI will eventually appear at: https://ci.nodejs.org/job/node-test-pull-request/18444/

@richardlau
Copy link
Member

Please could you add benchmark to https://github.com/nodejs/node/blob/master/test/README.md?

benchmark will still be run as part of the main CI unless it's excluded in

node/tools/test.py

Lines 1495 to 1506 in 83410d6

# these suites represent special cases that should not be run as part of the
# default JavaScript test-run, e.g., internet/ requires a network connection,
# addons/ requires compilation.
IGNORED_SUITES = [
'addons',
'addons-napi',
'doctool',
'internet',
'pummel',
'tick-processor',
'v8-updates'
]

@Trott
Copy link
Member Author

Trott commented Nov 9, 2018

benchmark will still be run as part of the main CI unless it's excluded in

Just saw that in the auto-run LinuxONE test and added a fixup commit.

Will add benchmark to the README. Thanks!

@richardlau
Copy link
Member

(If someone on Windows could test vcbuild test-benchmark, that would be great.)

If no one else does I'll try to later when I'm in the office. I suspect test-benchmark-napi.js will fail as I don't think we build the required addon on Windows (yet).

@Trott
Copy link
Member Author

Trott commented Nov 9, 2018

If no one else does I'll try to later when I'm in the office. I suspect test-benchmark-napi.js will fail as I don't think we build the required addon on Windows (yet).

That test is skipped on Windows precisely because we don't build the addon there. So I think it should work.

@Trott
Copy link
Member Author

Trott commented Nov 9, 2018

Merge conflict resolved.

CI: https://ci.nodejs.org/job/node-test-pull-request/18462/ (I think...check Parameters once it's actually launched)

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Thank you!!

@richardlau
Copy link
Member

(If someone on Windows could test vcbuild test-benchmark, that would be great.)

Currently building ⌛️.

Small nit: add test-benchmark to vcbuild.bat help output:
https://github.com/nodejs/node/blob/674a2c9b3d0a6edb7dbcac140215877fa17f1197/vcbuild.bat#L638

@refack
Copy link
Contributor

refack commented Nov 9, 2018

Move benchmark tests (which are slow) out of the main test suite. We can
hopefully add them to node-daily-master so that they are still run daily
on CI.

There is another less "churny" way to accomplish this (not exactly, but at least the goal of not running them with every CI job)- Keep them all in place, and just add them to https://github.com/nodejs/node/blob/master/test/root.status under [true] as SLOW.

Another option is to name the new suite sanity or dogfood or tools-smoke-test or something more generic, that we could maybe move move test that do sanity testing of tools, and not unit/functional testing of core (for example test-npm, test-docs-gen)

@richardlau
Copy link
Member

(If someone on Windows could test vcbuild test-benchmark, that would be great.)

Currently building ⌛️.

Unfortunately for some reason I'm unable to build master/v11.x-staging (nothing to do with this PR). v10.x-staging does build, and I was able to validate that the vcbuild changes start the benchmark tests (I get two failures, but I'm almost certain that's because it's running against a 10.x binary rather than one from master).

@Trott
Copy link
Member Author

Trott commented Nov 9, 2018

@refack I think having a make test-benchmark task is a feature worth having. And in general, I think we'd benefit from organizing our tests a bit more into functional directories like that. You have a point on the churn, but I'm OK with it.

Although your comment reminds me to remove these tests from root.status! I checked test/*/*.status for these files but forgot about root.status!

@Trott
Copy link
Member Author

Trott commented Nov 9, 2018

@refack
Copy link
Contributor

refack commented Nov 9, 2018

I think having a make test-benchmark task is a feature worth having. And in general, I think we'd benefit from organizing our tests a bit more into functional directories like that. You have a point on the churn, but I'm OK with it.

+1 on "functional directories", I'm just not sure about test-benchmarks-* being cohesive enough... But that can be refactored in the future..

@Trott
Copy link
Member Author

Trott commented Nov 10, 2018

Rebased to resolve a merge conflict. CI: https://ci.nodejs.org/job/node-test-pull-request/18474/

@codebytere
Copy link
Member

codebytere commented Dec 14, 2018

@Trott do you think this could be backported to v10.x?

Trott added a commit to Trott/io.js that referenced this pull request Dec 14, 2018
Move benchmark tests (which are slow) out of the main test suite. We can
hopefully add them to node-daily-master so that they are still run daily
on CI.

PR-URL: nodejs#24265
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member Author

Trott commented Dec 14, 2018

@Trott do you think this could be backported to v10.x?

v10.x backport in #25049

MylesBorins pushed a commit that referenced this pull request Dec 21, 2018
Move benchmark tests (which are slow) out of the main test suite. We can
hopefully add them to node-daily-master so that they are still run daily
on CI.

Backport-PR-URL: #25049
PR-URL: #24265
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 26, 2018
Move benchmark tests (which are slow) out of the main test suite. We can
hopefully add them to node-daily-master so that they are still run daily
on CI.

Backport-PR-URL: #25049
PR-URL: #24265
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Jan 4, 2019
@Trott Trott mentioned this pull request Jul 17, 2019
@Trott Trott deleted the benchmarks branch January 13, 2022 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. benchmark Issues and PRs related to the benchmark subsystem. build Issues and PRs related to build files or the CI. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants