test: adding test for configure ninja by patrickhousley · Pull Request #30033 · nodejs/node · GitHub

Updated the tooltest target to run unittest module
Renamed test/tools/test-js2c.py to be discoverable by unittest module
Added test class for configure shell script
Added a test to ensure configure script exits with status code zero
when passed the --ninja flag

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

@nodejs/build-files @nodejs/testing This could use some reviews.

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

This is awesome work! Thanks. Line 21 should be a backslash, not a slash? One minor nit. PEP8 strongly discourages the use of backslashes in Python code because one whitespace to the right of the backslash breaks the script on a change that is invisible to the reader. Could we instead use parens:

if (__name__ == '__main__' and
    sys.platform in ('linux', 'linux2', 'darwin', 'cygwin')):

Updated and fixed a few other pep8 warnings.

- Updated the tooltest target to run unittest module - Renamed test/tools/test-js2c.py to be discoverable by unittest module - Added test class for `configure` shell script - Added a test to ensure `configure` script exits with status code zero when passed the `--ninja` flag Closes nodejs#29415

@patrickhousley The failing Travis CI test was a fluke. It passed quickly when it was rerun.

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

This test might have helped us to find #30218 and #30220 sooner.

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

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

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

CI: https://ci.nodejs.org/job/node-test-pull-request/26376/ ✔️

- Updated the tooltest target to run unittest module - Renamed test/tools/test-js2c.py to be discoverable by unittest module - Added test class for `configure` shell script - Added a test to ensure `configure` script exits with status code zero when passed the `--ninja` flag Closes: #29415 PR-URL: #30033 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Christian Clauss <cclauss@me.com>

Landed in 85dd9e8.

Thanks for the contribution! 🎉

This seems like it now always runs ./configure --ninja as part of a regular make test. make test should not affect configuration flags. I’ll open a revert.

This reverts commit 85dd9e8. `make test` should never change the current set of `configure` flags. Refs: nodejs#30033

This reverts commit 85dd9e8. `make test` should never change the current set of `configure` flags. Refs: #30033 PR-URL: #30295 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

@patrickhousley I’m sorry that this needed to be reverted – I think what we need for #29415 is a new job in our CI matrix, not something that we could really make part of our regular test suite that developers run locally

@addaleax Is your objection to the addition of test/tools/test_configure.py?

Would we be OK to create a new PR that redoes the change to Makefile and the rename test/tools/{test-js2c.py => test_js2c.py}?

@cclauss The issue is that the test added by this PR reran configure which overwrites the config.gypi used to build Node.js with a potentially different set of options (in this case --ninja).

@cclauss Yeah, I don’t have any objections to the rename/Makefile changes – other than that, 👍 to what @richardlau said.

Hi @patrickhousley, Would you be willing to make a new pull request that just makes the changes to Makefile and test-js2c.py? Or would you like me to do that?

Co-authored-by: @patrickhousley Fixes to Python tests to ensure that the following all pass: 1. __python2 -m pytest ./test ./tools__ # 30 tests pass 2. __python3 -m pytest ./test ./tools__ # 30 tests pass 3. __python2 -m unittest discover -s ./test/tools__ # 1 test passes 4. __python3 -m unittest discover -s ./test/tools__ # 1 test passes 5. __PYTHON=python2 make tooltest__ # 1 test passes 6. __PYTHON=python3 make tooltest__ # 1 test passes This is a subset of nodejs#30033

Co-authored-by: @patrickhousley Fixes to Python tests to ensure that the following all pass: 1. __python2 -m pytest ./test ./tools__ # 30 tests pass 2. __python3 -m pytest ./test ./tools__ # 30 tests pass 3. __python2 -m unittest discover -s ./test/tools__ # 1 test passes 4. __python3 -m unittest discover -s ./test/tools__ # 1 test passes 5. __PYTHON=python2 make tooltest__ # 1 test passes 6. __PYTHON=python3 make tooltest__ # 1 test passes This is a subset of #30033 PR-URL: #30340 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: David Carlier <devnexen@gmail.com>

- Updated the tooltest target to run unittest module - Renamed test/tools/test-js2c.py to be discoverable by unittest module - Added test class for `configure` shell script - Added a test to ensure `configure` script exits with status code zero when passed the `--ninja` flag Closes: #29415 PR-URL: #30033 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Christian Clauss <cclauss@me.com>

This reverts commit 85dd9e8. `make test` should never change the current set of `configure` flags. Refs: #30033 PR-URL: #30295 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

Co-authored-by: @patrickhousley Fixes to Python tests to ensure that the following all pass: 1. __python2 -m pytest ./test ./tools__ # 30 tests pass 2. __python3 -m pytest ./test ./tools__ # 30 tests pass 3. __python2 -m unittest discover -s ./test/tools__ # 1 test passes 4. __python3 -m unittest discover -s ./test/tools__ # 1 test passes 5. __PYTHON=python2 make tooltest__ # 1 test passes 6. __PYTHON=python3 make tooltest__ # 1 test passes This is a subset of #30033 PR-URL: #30340 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: David Carlier <devnexen@gmail.com>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 18, 2019

patrickhousley force-pushed the feature/test-configure branch from 10e8bac to 8b9230f Compare October 18, 2019 23:19

sam-github requested a review from cclauss October 23, 2019 21:48

sam-github approved these changes Oct 23, 2019

View reviewed changes

cclauss approved these changes Oct 24, 2019

View reviewed changes

patrickhousley force-pushed the feature/test-configure branch from 8b9230f to 08b5e13 Compare October 25, 2019 01:52

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 25, 2019

cclauss mentioned this pull request Oct 27, 2019

build: ongoing list of actions for Python 3 compatibility #25789

Closed

63 tasks

Trott closed this Nov 6, 2019

addaleax mentioned this pull request Nov 6, 2019

Tests do not test ./configure --ninja #29415

Closed

addaleax added a commit to addaleax/node that referenced this pull request Nov 6, 2019

Revert "test: test configure ninja"

87aac3f

This reverts commit 85dd9e8. `make test` should never change the current set of `configure` flags. Refs: nodejs#30033

addaleax mentioned this pull request Nov 6, 2019

Revert "test: test configure ninja" #30295

Closed

3 tasks

cclauss mentioned this pull request Nov 8, 2019

test: Fix Python unittests in ./test and ./tools #30340

Closed

2 tasks

BridgeAR mentioned this pull request Nov 19, 2019

v13.2.0 proposal #30547

Merged

targos added dont-land-on-v10.x labels Dec 1, 2019

test: adding test for configure ninja by patrickhousley · Pull Request #30033 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: adding test for configure ninja - #30033

test: adding test for configure ninja#30033patrickhousley wants to merge 1 commit intonodejs:masternodejs/node:masterfrom patrickhousley:feature/test-configurepatrickhousley/node:feature/test-configureCopy head branch name to clipboard

Conversation

patrickhousley commented Oct 18, 2019

Checklist

Uh oh!

Trott commented Oct 22, 2019

Uh oh!

nodejs-github-bot commented Oct 23, 2019

Uh oh!

cclauss Oct 24, 2019 • 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!

patrickhousley Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

cclauss commented Oct 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 25, 2019

Uh oh!

cclauss commented Nov 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 3, 2019

Uh oh!

nodejs-github-bot commented Nov 3, 2019

Uh oh!

nodejs-github-bot commented Nov 3, 2019

Uh oh!

nodejs-github-bot commented Nov 5, 2019 • edited by Trott Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Nov 6, 2019

Uh oh!

addaleax commented Nov 6, 2019

Uh oh!

addaleax commented Nov 6, 2019

Uh oh!

cclauss commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardlau commented Nov 6, 2019

Uh oh!

addaleax commented Nov 6, 2019

Uh oh!

cclauss commented Nov 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

test: adding test for configure ninja#30033
patrickhousley wants to merge 1 commit into
nodejs:masterfrom
patrickhousley:feature/test-configure

cclauss Oct 24, 2019 •
edited
Loading

cclauss commented Oct 25, 2019 •
edited
Loading

cclauss commented Nov 2, 2019 •
edited
Loading

nodejs-github-bot commented Nov 5, 2019 •
edited by Trott
Loading

cclauss commented Nov 6, 2019 •
edited
Loading