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: adding test for configure ninja #30033

Closed

Conversation

patrickhousley
Copy link
Contributor

  • 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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • 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 Oct 18, 2019
@Trott
Copy link
Member

Trott commented Oct 22, 2019

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

@nodejs-github-bot
Copy link
Collaborator

)

if __name__ == '__main__' and\
sys.platform in ['linux', 'linux2', 'darwin', 'cygwin']:
Copy link
Contributor

@cclauss cclauss Oct 24, 2019

Choose a reason for hiding this comment

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

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')):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
@cclauss
Copy link
Contributor

cclauss commented Oct 25, 2019

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

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

@cclauss
Copy link
Contributor

cclauss commented Nov 2, 2019

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 5, 2019

Trott pushed a commit that referenced this pull request Nov 6, 2019
- 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 <[email protected]>
Reviewed-By: Christian Clauss <[email protected]>
@Trott
Copy link
Member

Trott commented Nov 6, 2019

Landed in 85dd9e8.

Thanks for the contribution! 🎉

@Trott Trott closed this Nov 6, 2019
@addaleax
Copy link
Member

addaleax commented Nov 6, 2019

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.

addaleax added a commit to addaleax/node that referenced this pull request Nov 6, 2019
This reverts commit 85dd9e8.

`make test` should never change the current set of `configure`
flags.

Refs: nodejs#30033
addaleax added a commit that referenced this pull request Nov 6, 2019
This reverts commit 85dd9e8.

`make test` should never change the current set of `configure`
flags.

Refs: #30033

PR-URL: #30295
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
@addaleax
Copy link
Member

addaleax commented Nov 6, 2019

@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

@cclauss
Copy link
Contributor

cclauss commented Nov 6, 2019

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

@richardlau
Copy link
Member

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

@addaleax
Copy link
Member

addaleax commented Nov 6, 2019

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

@cclauss
Copy link
Contributor

cclauss commented Nov 7, 2019

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?

cclauss added a commit to cclauss/node that referenced this pull request Nov 9, 2019
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
cclauss added a commit that referenced this pull request Nov 11, 2019
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 <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
- 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 <[email protected]>
Reviewed-By: Christian Clauss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
This reverts commit 85dd9e8.

`make test` should never change the current set of `configure`
flags.

Refs: #30033

PR-URL: #30295
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
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 <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Nov 19, 2019
targos pushed a commit that referenced this pull request Dec 1, 2019
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 <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
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 <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
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. build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests do not test ./configure --ninja
8 participants