test, build: add test-ci wrappers #12874

refack · 2017-05-06T18:32:54Z

Ref: #12830 (comment)
Fixes: #12771

Added scripts that use the same args as the CI for use with single files or suites

Need help reviewing the change in Makefile whether it's safe as it bypasses the PYTHON var in config.mk

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

refack · 2017-05-06T18:33:25Z

/cc @nodejs/build @nodejs/python

refack · 2017-05-06T18:34:23Z

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

addaleax · 2017-05-06T18:37:49Z

tools/test-ci.sh

@@ -0,0 +1,3 @@
+pushd `dirname $0`/.. > /dev/null


Please add a shebang line at the top of this file.

Also, just use cd, pushd isn’t a common feature of all POSIX shells (and there’s no need to do the redirect).

addaleax · 2017-05-06T18:38:38Z

tools/test-ci.sh

@@ -0,0 +1,3 @@
+pushd `dirname $0`/.. > /dev/null
+`which python` tools/test.py -J --mode=release $*


Regarding your question in the PR description, you may want to look into using $PYTHON here (and possible exporting that in the Makefile beforehand, I’m not sure)

Read, and found the way, thanks

addaleax · 2017-05-06T18:40:01Z

Makefile


 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


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

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

addaleax · 2017-05-06T18:41:04Z

vcbuild.ps1

+
+:exit
+if not defined DISTTYPEDIR $DISTTYPEDIR=%DISTTYPE%
+goto :EOF


Can I suggest adding this file in a second commit (or PR)? The changes don’t seem related?

* Replace explicit use in `Makefile` and `vcbuild.bat`

refack · 2017-05-06T19:18:54Z

@addaleax the .ps1 file leaked in, now replaced with the actual vcbuild.bat

addaleax

This only seems to add abstraction that we don’t really need, so I’m -0 on it

addaleax · 2017-05-06T19:29:58Z

tools/test-ci.sh

@@ -0,0 +1,4 @@
+#!/bin/bash
+cd `dirname $0`/.. > /dev/null
+if [ -z ${PYTHON+x} ]; then export PYTHON=`which python`; fi


if [ -z "$PYTHON" ]

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

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

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 🤷‍♀️

addaleax · 2017-05-06T19:30:19Z

tools/test-ci.cmd

@@ -0,0 +1,3 @@
+pushd %~dp0\..
+python tools\test.py -J --mode=release %*
+popd


just curious, does the Windows cmd actually require this?

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

addaleax · 2017-05-06T19:30:23Z

tools/test-ci.sh

+#!/bin/bash
+cd `dirname $0`/.. > /dev/null
+if [ -z ${PYTHON+x} ]; then export PYTHON=`which python`; fi
+${PYTHON} tools/test.py -J --mode=release $*


"$PYTHON" or "${PYTHON}"

mscdex · 2017-05-06T19:34:53Z

I think I'm -1 on this. I'd rather not have command line arguments hidden. This would require you to go and look at the contents of the script to see what's currently being passed if you run into unexpected behavior, etc.

addaleax · 2017-05-06T19:37:55Z

One more thing I just noticed: The wrapper scripts don’t forward errors from the test runner properly

refack · 2017-05-06T19:40:10Z

One more thing I just noticed: The wrapper scripts don’t forward errors from the test runner properly

you mean ERRORLEVEL / exit code?

refack · 2017-05-06T19:43:47Z

I'd rather not have command line arguments hidden.

I agree it's a mixed bag...
On the one hand you get standardization (as the thread in #12830 (comment) discusses) and gives one point of change
On the other hand there is the abstraction...

Something to think about....

refack · 2017-05-06T19:55:32Z

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

aqrln · 2017-05-06T20:25:48Z

Makefile

@@ -12,6 +12,7 @@ LOGLEVEL ?= silent
 OSTYPE := $(shell uname -s | tr '[A-Z]' '[a-z]')
 COVTESTS ?= test
 GTEST_FILTER ?= "*"
+export PYTHON


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=....

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

Verified. It does not leak.

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

gibfahn · 2017-05-06T20:37:19Z

To continue from #12830 (comment)

It'll take $*, so it's useful for single files, or single suites.
I don't know about make test-ci but vcbuild test-ci builds first (with Feet of clay), very demotivating.

I get the problem that this is trying to solve, but I think the downside of adding an extra level of indirection (and another file that people have to know about) outweighs the benefit of having a standard way to run things, especially as the CI runs tools/test.py with almost the default options. I'd rather either default to -J or drop my objection in #12830 (comment) than do this.

The windows build sounds like a problem, and I think it's definitely one we should look at. Does it still rebuild if you do vcbuild nobuild test-ci (from what I recall you have to tell vcbuild not to rebuild, it can't work it out itself)? If yes then we should fix that.

On another note if you've got a WIP version of vcbuild.ps1 I'd be interested in seeing it in PR form, and it might save time and effort to get feedback from Windows Node people earlier in the design process.

refack · 2017-05-06T20:49:23Z

I get the problem that this is trying to solve, but I think the downside of adding an extra level of indirection

Actually I kind of started liking this, since it's self documenting code: "How does the CI call test.py" -> "as is written in test-ci.sh"

But If this gets -1 votes, I have no problem shelving it.

Although I'd ask you all to think about it some more, less then as "another tool" but more as "refactoring out" all those same calls from the files I touched.

@gibfahn as for the WIP vcbuild.ps1 I've started with @joaocgreis's advice to try to 1-1 convert the .bat file (which is mostly busy work, but it keeps me out of trouble 😉 ) https://gist.github.com/refack/969ecc6642981bfd7caef7d948d78076

gibfahn · 2017-05-07T09:06:38Z

Makefile

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


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?

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 ✊

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

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 $*

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

@refack git is not a requirement for testing, tho

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

@addaleax for the whole concept?

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.

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

thefourtheye · 2017-05-08T08:26:27Z

tools/test-ci.sh

@@ -0,0 +1,5 @@
+#!/bin/bash


#!/usr/bin/env bash

would be more portable, right?

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

does sh have if [ -z?

refack · 2017-07-22T14:59:30Z

Not really need

nodejs-github-bot added build Issues and PRs related to build files or the CI. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform. labels May 6, 2017

refack added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label May 6, 2017

refack mentioned this pull request May 6, 2017

doc: Change test option at STEP 5: Test in CONTRIBUTING.md #12830

Closed

2 tasks

addaleax reviewed May 6, 2017

View reviewed changes

refack force-pushed the test-ci-wrappers branch from 7649cde to c8a62a0 Compare May 6, 2017 18:56

build, tools: add test-ci scripts

7ff770c

* Replace explicit use in `Makefile` and `vcbuild.bat`

refack force-pushed the test-ci-wrappers branch from c8a62a0 to 7ff770c Compare May 6, 2017 18:59

[squash] use exported PYTHON

4fa2a44

change documentation (to finish the PR's story)

964b2a1

refack changed the title ~~test, build, Test ci wrappers~~ test, build: add test-ci wrappers May 6, 2017

addaleax reviewed May 6, 2017

View reviewed changes

[squash]explicit path

7d757da

refack added 2 commits May 6, 2017 15:46

[squash] handle errorlevel

f666543

[squash] wrap strings

1860972

[squash] no +x

e9b2d11

aqrln reviewed May 6, 2017

View reviewed changes

gibfahn reviewed May 7, 2017

View reviewed changes

thefourtheye reviewed May 8, 2017

View reviewed changes

refack mentioned this pull request May 10, 2017

create public nodejs/collaborators repo nodejs/TSC#243

Closed

refack force-pushed the test-ci-wrappers branch from b45c132 to e9b2d11 Compare May 16, 2017 19:29

refack closed this Jul 22, 2017

		@@ -0,0 +1,3 @@
		pushd `dirname $0`/.. > /dev/null
		`which python` tools/test.py -J --mode=release $*

test, build: add test-ci wrappers #12874

test, build: add test-ci wrappers #12874

Conversation

refack commented May 6, 2017 • edited Loading

Checklist

Affected core subsystem(s)

refack commented May 6, 2017

refack commented May 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented May 6, 2017

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented May 6, 2017

addaleax commented May 6, 2017

refack commented May 6, 2017

refack commented May 6, 2017

refack commented May 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn commented May 6, 2017

refack commented May 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn May 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jul 22, 2017

refack commented May 6, 2017 •

edited

Loading

gibfahn May 8, 2017 •

edited

Loading