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

build: restore $(CI_JS_SUITES) to test-ci-js #16132

Merged
merged 1 commit into from
Oct 12, 2017

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 10, 2017

$(CI_JS_SUITES) was dropped during the last commit by a mistake

Fixes: #16129
Fixes: nodejs/build#910

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

build

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 10, 2017
@refack
Copy link
Contributor Author

refack commented Oct 10, 2017

CI on arm-fanned (only job that's affected): https://ci.nodejs.org/job/node-test-commit-arm-fanned/11554/ (fragments are back to 270 * 7)

@refack
Copy link
Contributor Author

refack commented Oct 10, 2017

/cc @bcoe 😉

@bcoe
Copy link
Contributor

bcoe commented Oct 10, 2017

@refack looks good to me, I think I screwed that up in a merge (since that was the whole darn point of pulling things into the default helper).

Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

😩 sorry about that.

@refack
Copy link
Contributor Author

refack commented Oct 10, 2017

Now this also fixes #16129

@refack
Copy link
Contributor Author

refack commented Oct 10, 2017

CI: https://ci.nodejs.org/job/node-test-commit/13005/

Intending to land tomorrow morning if there are now objections, since we're missing test coverage of the addons.

@@ -372,7 +372,7 @@ test-ci: | clear-stalled build-addons build-addons-napi
out/Release/cctest --gtest_output=tap:cctest.tap
$(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \
--mode=release --flaky-tests=$(FLAKY_TESTS) \
$(TEST_CI_ARGS) $(CI_ASYNC_HOOKS) $(CI_JS_SUITES) known_issues
$(TEST_CI_ARGS) $(CI_ASYNC_HOOKS) $(CI_JS_SUITES) $(CI_NATIVE_SUITES) known_issues
Copy link
Contributor

Choose a reason for hiding this comment

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

something I just noticed, could you switch:

CI_NATIVE_SUITES := addons addons-napi

to

CI_NATIVE_SUITES ?= addons addons-napi

while you're at it; that would allow this set of tests to be more easily turned off when running coverage locally. tldr; I think this file got slightly screwed up during rebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM and + 1 for expediting so we don't run too long with missing tests.

suites dropped during the last commit by a typo
restore $(CI_JS_SUITES) to test-ci-js
restore $(CI_NATIVE_SUITES) for `test-ci`
and var assignment tweak
@refack refack force-pushed the small-makefile-fix branch from e3614bd to fc38604 Compare October 12, 2017 14:50
@refack refack merged commit fc38604 into nodejs:master Oct 12, 2017
refack added a commit to refack/node that referenced this pull request Oct 12, 2017
suites dropped during the last commit by a typo
restore $(CI_JS_SUITES) to test-ci-js
restore $(CI_NATIVE_SUITES) for `test-ci`
and var assignment tweak

PR-URL: nodejs#16132
Fixes: nodejs#16129
Fixes: nodejs/build#910
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin E. Coe <ben@npmjs.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
suites dropped during the last commit by a typo
restore $(CI_JS_SUITES) to test-ci-js
restore $(CI_NATIVE_SUITES) for `test-ci`
and var assignment tweak

PR-URL: nodejs/node#16132
Fixes: nodejs/node#16129
Fixes: nodejs/build#910
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin E. Coe <ben@npmjs.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
targos pushed a commit that referenced this pull request Oct 18, 2017
suites dropped during the last commit by a typo
restore $(CI_JS_SUITES) to test-ci-js
restore $(CI_NATIVE_SUITES) for `test-ci`
and var assignment tweak

PR-URL: #16132
Fixes: #16129
Fixes: nodejs/build#910
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin E. Coe <ben@npmjs.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
targos pushed a commit that referenced this pull request Oct 18, 2017
suites dropped during the last commit by a typo
restore $(CI_JS_SUITES) to test-ci-js
restore $(CI_NATIVE_SUITES) for `test-ci`
and var assignment tweak

PR-URL: #16132
Fixes: #16129
Fixes: nodejs/build#910
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin E. Coe <ben@npmjs.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins
Copy link
Contributor

@refack should we backport to 6.x?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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.

CI: make test-ci-js got broke and we didn't notice Coverage drop in nightly runs
7 participants