Skip to content

build: do not run benchmark tests on 'make test' #34434

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 8 commits into from
Jul 22, 2020

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 20, 2020

Looks like we've been running benchmark tests locally with make test. Whoops. Let's not do that.

Fixes: #34427

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 Jul 20, 2020
@Trott
Copy link
Member Author

Trott commented Jul 20, 2020

@nodejs/build-files @addaleax @rickyes

Copy link
Contributor

@rickyes rickyes left a comment

Choose a reason for hiding this comment

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

LGTM.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 20, 2020
@nodejs-github-bot
Copy link
Collaborator

Comment on lines +301 to +302
$(JS_SUITES) \
$(NATIVE_SUITES)
Copy link
Member

Choose a reason for hiding this comment

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

FWIW the reason we split tests into CI_JS_SUITES and CI_NATIVE_SUITES was that to accommodate the arm-fanned setup we split the tests into test-ci-native (requires a build toolchain set up to build the native addons) and test-ci-js (which "should not use a native compiler at all" (see comment in Makefile)).

For testing outside of the CI we don't have the same split in Makefile test targets, so you could optionally have a single make variable instead of two.

@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 21, 2020
PR-URL: nodejs#34420
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 22, 2020

puzpuzpuz and others added 3 commits July 22, 2020 16:20
PR-URL: nodejs#34048
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Fixes: nodejs#34242

PR-URL: nodejs#34417
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Otherwise the build would fail with
`./configure --experimental-quic --ninja` as the list of per-Environment
values would not match and the code cache builder would not generate
code cache for the quic JS sources. This is more or less a band-aid -
a proper fix would be to aggregate these flags into something
that can be included by all these different binary targets.
See nodejs#31074.

PR-URL: nodejs#34454
Fixes: nodejs#34435
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
addaleax and others added 4 commits July 22, 2020 07:30
This prevents accidental resource leaks when terminating or exiting
Worker that own FDs opened through `fs.open()`.

Refs: nodejs#34303

PR-URL: nodejs#34394
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
The "Labels" section of doc/guides/onboarding-extras.md was missing a
reference to the `author-ready` label. This commit adds a description
similar to the definition found in doc/guides/collaborator-guide.md
with the goal of making it easier for new contributors to find labels
info all in one place.

PR-URL: nodejs#34381
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Notable changes:

deps:
  * upgrade npm to 6.14.6 (claudiahdz) [nodejs#34246](nodejs#34246)
  * update node-inspect to v2.0.0 (Jan Krems) [nodejs#33447](nodejs#33447)
  * uvwasi: cherry-pick 9e75217 (Colin Ihrig) [nodejs#33521](nodejs#33521)

PR-URL: nodejs#34343
Fixes: nodejs#34427

PR-URL: nodejs#34434
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Jul 22, 2020

Landed in 3caa2e2

@Trott Trott merged commit 3caa2e2 into nodejs:master Jul 22, 2020
@Trott Trott deleted the no-bench-local branch July 22, 2020 17:18
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
Fixes: #34427

PR-URL: #34434
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
ruyadorno pushed a commit that referenced this pull request Jul 28, 2020
Fixes: #34427

PR-URL: #34434
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Jul 28, 2020
ruyadorno pushed a commit that referenced this pull request Jul 28, 2020
Fixes: #34427

PR-URL: #34434
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
ruyadorno pushed a commit that referenced this pull request Jul 29, 2020
Fixes: #34427

PR-URL: #34434
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
Fixes: #34427

PR-URL: #34434
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
Fixes: #34427

PR-URL: #34434
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Local run test failed: benchmark/test-benchmark-napi