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.

Sign up for GitHub

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

Jump to bottom

test, build: add test-ci wrappers #12874

Closed
wants to merge 7 commits into from
Closed

test, build: add test-ci wrappers #12874

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7ff770c
build, tools: add `test-ci` scripts
refack May 6, 2017
4fa2a44
[squash] use exported PYTHON
refack May 6, 2017
964b2a1
change documentation (to finish the PR's story)
refack May 6, 2017
7d757da
[squash]explicit path
refack May 6, 2017
f666543
[squash] handle errorlevel
refack May 6, 2017
1860972
[squash] wrap strings
refack May 6, 2017
e9b2d11
[squash] no +x
refack May 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,10 +169,17 @@ C++ linting.

If you are updating tests and just want to run a single test to check it:

```text
$ python tools/test.py -v --mode=release parallel/test-stream2-transform
```console
$ ./tools/test-ci.sh parallel/test-stream2-transform
```

Windows:

```console
> tools\test-ci parallel\test-stream2-transform
```


You can usually run tests directly with node:

```text
Expand Down
8 changes: 5 additions & 3 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ LOGLEVEL ?= silent
OSTYPE := $(shell uname -s | tr '[A-Z]' '[a-z]')
COVTESTS ?= test
GTEST_FILTER ?= "*"
export PYTHON
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to export it? Modifying environment variables as a side effect of make may be an unexpected behavior. I'd rather require users to either export it explicitly or prepend each command with PYTHON=....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I read in the Makefile man it is only "exported" to subshells, no the caller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Verified. It does not leak.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, okay. If that's the case, then fine.


ifdef JOBS
PARALLEL_ARGS = -j $(JOBS)
Expand Down Expand Up @@ -43,6 +44,7 @@ NODE_EXE = node$(EXEEXT)
NODE ?= ./$(NODE_EXE)
NODE_G_EXE = node_g$(EXEEXT)
NPM ?= ./deps/npm/bin/npm-cli.js
TEST_CI = ./tools/test-ci.sh
Copy link
Member

Choose a reason for hiding this comment

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

If you made this something like:

TEST_CI = tools/test.py --mode=release -J

then wouldn't you get the same deduplication without the indirection of a separate file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But what about vcbuild.bat and the case of #12830 & #12771
I was actually thinking of refactoring ALL the similar cases into tools/make to help alleviate #12425
But on the other hand Makefile can almost be read like a configuration file...

Anyway we should all move to ninja with one node.ninja for all ✊

Copy link
Member

@gibfahn gibfahn May 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm pretty sure --mode=release is the default, so if we made -J the default as well then we definitely wouldn't need this indirection.

But what about vcbuild.bat

This doesn't help with that though, you still have the tools/test.py command duplicated in Windows and Unix files.

and the case of #12830 & #12771

I think people should run tools/test.py directly, this discourages them from doing that.

Anyway we should all move to ninja with one node.ninja for all ✊

SGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But on the other hand Makefile can almost be read like a configuration file...

What about creating a mechanism with an explicit configuration file, that all script derive from...
(could be a Template filled by GYP)
something like:

test-cl:
  "${PYTHON}" tools/test.py -J --mode=release $*
test-seq:
  "${PYTHON}" tools/test.py --mode=release $*
test-deopt:
  ${test-cl} --check-deopts $*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or hooking it into git (since it's already a requirement, and is platform uniform), then we could do

$ git tool test-cl parallel/test-stream2-transform
$ git tool lint
$ git tool benchmark buffers/buffer-tostring.js len=1024

Copy link
Member

Choose a reason for hiding this comment

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

@refack git is not a requirement for testing, tho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@refack git is not a requirement for testing, tho

so a dual runner, git tool and node tool. One for bootstrapping, one for post-build tooling.
Could even throw in a python tool for the extreme minimalists...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@addaleax for the whole concept?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I don’t think we should duplicate the same functionality over 2 or even 3 implementations.

If you want honest advice: Close this PR, or at least let it rest until you find somebody that thinks this is a good idea. Multiple people here have expressed doubt about the usefulness of this, and in a consensus-seeking model that’s a sign that it’s just not going to happen. And so the chances are, the more work you put into this, the more frustrated you’re going to be when the PR just stalls and doesn’t go anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want honest advice: Close this PR

Yeah I've already put it on the back burner #12874 (comment)

Just kicking around the idea...


# Flags for packaging.
BUILD_DOWNLOAD_FLAGS ?= --download=all
Expand Down Expand Up @@ -195,18 +197,18 @@ test: all
$(MAKE) build-addons
$(MAKE) build-addons-napi
$(MAKE) cctest
$(PYTHON) tools/test.py --mode=release -J \
$(TEST_CI) \
doctool inspector known_issues message pseudo-tty parallel sequential $(CI_NATIVE_SUITES)
$(MAKE) lint

test-parallel: all
$(PYTHON) tools/test.py --mode=release parallel -J
$(TEST_CI) parallel

test-valgrind: all
$(PYTHON) tools/test.py --mode=release --valgrind sequential parallel message

test-check-deopts: all
$(PYTHON) tools/test.py --mode=release --check-deopts parallel sequential -J
$(TEST_CI) --check-deopts parallel sequential
Copy link
Member

Choose a reason for hiding this comment

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

I have to admit I don’t quite get what the goal of these changes is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Standardizing the calls to python tools\test.py
I admit that I'm not sure of the benefit of the changes in Makefile, especially if there is risk of breakage


# Implicitly depends on $(NODE_EXE). We don't depend on it explicitly because
# it always triggers a rebuild due to it being a .PHONY rule. See the comment
Expand Down
3 changes: 3 additions & 0 deletions tools/test-ci.cmd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
pushd %~dp0\..
python tools\test.py -J --mode=release %*
popd
Copy link
Member

Choose a reason for hiding this comment

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

just curious, does the Windows cmd actually require this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it doesn't spawn a subshell so it cd the caller

4 changes: 4 additions & 0 deletions tools/test-ci.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

#!/usr/bin/env bash

would be more portable, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yep. But here it should be #!/bin/sh anyway, not bash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does sh have if [ -z?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

cd `dirname $0`/.. > /dev/null
if [ -z ${PYTHON+x} ]; then export PYTHON=`which python`; fi
Copy link
Member

Choose a reason for hiding this comment

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

if [ -z "$PYTHON" ]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stole from http://stackoverflow.com/questions/3601515/how-to-check-if-a-variable-is-set-in-bash

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but I think setting PYTHON='' should give the default behaviour. Also, setting PYTHON=python in the then case here should be just fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PYTHON=`which python` is for me, when running in WSL (Windows System for Linux) there's a bad behaviour where it'll try to run the windows python over the Ubuntu one 🤷‍♀️

${PYTHON} tools/test.py -J --mode=release $*
Copy link
Member

Choose a reason for hiding this comment

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

"$PYTHON" or "${PYTHON}"

9 changes: 6 additions & 3 deletions vcbuild.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,9 @@ if defined test_node_inspect goto node-test-inspect
goto node-tests

:node-check-deopts
python tools\test.py --mode=release --check-deopts parallel sequential -J
set "test_deopts_cmd=tools\test-ci.cmd --check-deopts parallel sequential"
echo running '%test_deopts_cmd%'
call %test_deopts_cmd%
if defined test_node_inspect goto node-test-inspect
goto node-tests

Expand All @@ -418,8 +420,9 @@ if "%config%"=="Debug" set test_args=--mode=debug %test_args%
if "%config%"=="Release" set test_args=--mode=release %test_args%
echo running 'cctest %cctest_args%'
"%config%\cctest" %cctest_args%
echo running 'python tools\test.py %test_args%'
python tools\test.py %test_args%
set "test_cmd=tools\test-ci.cmd %test_args%"
echo running '%test_cmd%'
call %test_cmd%
goto cpplint

:cpplint
Expand Down