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

jenkins: add node-test-commit-v8-linux script #1344

Closed
wants to merge 3 commits into from

Conversation

mmarchini
Copy link
Contributor

@mmarchini
Copy link
Contributor Author

Should be enough to run v8-updates tests on Ubuntu 16.04. @maclover7 PTAL

@mmarchini mmarchini force-pushed the node-test-v8-updates branch from 9a03fa2 to 6916a66 Compare June 14, 2018 22:07
@refack
Copy link
Contributor

refack commented Jun 14, 2018

@mmarchini the build phase of the job should be similar to a regular node-test-commit?

@mmarchini
Copy link
Contributor Author

Yes

@refack
Copy link
Contributor

refack commented Jun 14, 2018

So this is our build script

#!/bin/bash -xe

FLAKY_TESTS_MODE=run
if test $IGNORE_FLAKY_TESTS = "true"; then
  FLAKY_TESTS_MODE=dontcare
fi

echo FLAKY_TESTS_MODE=$FLAKY_TESTS_MODE

if [ -z ${JOBS+x} ]; then
  export JOBS=$(getconf _NPROCESSORS_ONLN)
fi

MAKE_ARGS="-j $JOBS"

if [ $(make -v | grep 'GNU Make 4' -c) -ne 0 ]; then
  MAKE_ARGS="$MAKE_ARGS --output-sync=target"
fi

exec_cmd=" \
  NODE_TEST_DIR=${HOME}/node-tmp \
  PYTHON=python \
  FLAKY_TESTS=$FLAKY_TESTS_MODE \
  make run-ci $MAKE_ARGS \
"

if test $nodes = "centos6-64-gcc48"; then
  exec_cmd=". /opt/rh/devtoolset-2/enable; $exec_cmd"
elif [[ "$nodes" =~ centos[67]-(arm)?64-gcc6 ]]; then
  exec_cmd=". /opt/rh/devtoolset-6/enable; $exec_cmd"
fi

echo $exec_cmd
V=1

$SHELL -xec "$exec_cmd"

I'm sure there is a ton of cross platform unneeded code there, but the tl;dr is we run make run-ci

@mmarchini
Copy link
Contributor Author

mmarchini commented Jun 14, 2018

Since there's no need to run all tests, is make run-ci CI_JS_SUITES="v8-updates" CI_NATIVE_SUITES="" CI_DOC="" acceptable?

diff --git a/jenkins/scripts/node-test-v8-updates.sh b/jenkins/scripts/node-test-v8-updates.sh
index 487d569..72da7aa 100755
--- a/jenkins/scripts/node-test-v8-updates.sh
+++ b/jenkins/scripts/node-test-v8-updates.sh
@@ -11,7 +11,4 @@ git clone https://github.com/nodejs/build.git
 
 . ./build/jenkins/scripts/node-test-commit-pre.sh
 
-./configure;
-make -j $(getconf NPROCESSORS_ONLN);
-
-python tools/test.py -p tap --logfile test.tap --mode=release v8-updates
+make run-ci CI_JS_SUITES="v8-updates" CI_NATIVE_SUITES="" CI_DOC=""

@refack
Copy link
Contributor

refack commented Jun 14, 2018

I'm tryin to think of the smoothest way to add this job since node-test-commit-v8-linux is not a multiphase job like node-test-commit...
I think I have an idea.

@refack refack self-assigned this Jun 14, 2018
@refack
Copy link
Contributor

refack commented Jun 14, 2018

@mmarchini I created a new job and given you edit perms: https://ci.nodejs.org/job/node-test-commit-linux-v8-updates/configure
Let's see if it works

@mmarchini
Copy link
Contributor Author

mmarchini commented Jun 14, 2018

Worked 😄

19:12:49 1..2
19:12:49 ok 1 v8-updates/test-postmortem-metadata
19:12:49   ---
19:12:49   duration_ms: 0.915
19:12:49   ...
19:12:49 ok 2 v8-updates/test-linux-perf
19:12:49   ---
19:12:49   duration_ms: 5.223
19:12:49   ...

@maclover7
Copy link
Contributor

@mmarchini Ahh, didn't fully understand what was needed here. In theory, you should just be able to utilize node-test-commit-custom-suites, I started a sample of what this could look like at https://ci.nodejs.org/job/node-test-commit-custom-suites/11

@mmarchini
Copy link
Contributor Author

@maclover7 thanks for pointing this out, but node-test-commit-custom-suites will run on rhel72-s390x and test/v8-updates should be run on Ubuntu 16.04 (otherwise the Linux perf test will be skipped).

@refack
Copy link
Contributor

refack commented Jun 15, 2018

I added node-test-commit-linux-v8-updates as a build step of node-test-commit-v8-linux. I don't think we used this feature before, let's see how it behaves.
https://ci.nodejs.org/job/node-test-commit-v8-linux/1427/

@maclover7
Copy link
Contributor

test/v8-updates should be run on Ubuntu 16.04

@mmarchini Ah true -- let me update that job configuration, and then we should be okay to use that (would rather try and reuse existing job pieces if possible than introduce new stuff, to avoid more complexity in Jenkins)

@refack
Copy link
Contributor

refack commented Jun 15, 2018

Well that gives weird results, every platform job triggers a linux-v8-updates jobs
https://ci.nodejs.org/job/node-test-commit-v8-linux/1427/nodes=benchmark,v8test=v8test/

Let me do some key mashing... 🤔

@refack
Copy link
Contributor

refack commented Jun 15, 2018

@mklebrasseur do you have a quick answer for this?

tl;dr:
we have an exsisting multi-configuration project (https://ci.nodejs.org/job/node-test-commit-v8-linux/), we want to add a parameterized sub job (https://ci.nodejs.org/job/node-test-commit-custom-suites) to it with minimal refactoring

@mklebrasseur
Copy link

@refack sure I can have a look. Do I need any additional access for those jobs to view the build steps. I am rather new to this project and have not got my grounds yet.

@refack
Copy link
Contributor

refack commented Jun 15, 2018

Do I need any additional access for those jobs to view the build steps.

Unfortunately, Yes... If you know how to enable "read only" view, that's also interesting.

Best I can do is a screenshot and the XML
https://gist.github.com/refack/05e9afb15c722ce2fece27bab620a1f2
node-test-commit-v8-linux Config [Jenkins].pdf

ATM I added the sub job as the last post-build-action.

@mklebrasseur
Copy link

Ok, thanks,

At first glance could use "when" to decide when to run it or not. I will review further and follow up. Thanks for the invite on the review :).

@refack
Copy link
Contributor

refack commented Jun 15, 2018

At first glance could use "when" to decide when to run it or not.

🔔 🔔 🔔
Well since the job is a "legacy" job and not pipeline, I'll use a conditional build step.

@mklebrasseur
Copy link

Yea, my mistake I was looking at the repo before with the use of some pipelines. I've never enjoyed having to use the UI to build out any builds. Shared libraries I found to be much easier and allow for granular access using git (as well as read only). We recently converted all our legacy builds to pipelines this month. It's very fresh in my head.

Conditional will work fine, also the use of a shared library would work too.

Apologies for the delayed response.

@refack
Copy link
Contributor

refack commented Jun 16, 2018

@mklebrasseur and @mmarchini I gave you read access to https://ci.nodejs.org/job/node-test-commit-v8-linux/configure
Please say you can see that. This is a test of a new plugin that allows read only access to joob configs.

@mklebrasseur
Copy link

@refack yes I can confirm that can see the configuration. No save options appear (y)

@mmarchini

This comment has been minimized.

@mmarchini
Copy link
Contributor Author

v8-updates tests are still not running for node-test-commit-v8-linux, even though settings seems to be correct:

image

image

Relevant Jenkins log:

05:53:51 Join notifier cannot find upstream project: node-test-commit-v8-linux/nodes=benchmark,v8test=v8test

https://ci.nodejs.org/job/node-test-commit-custom-suites/17/

@mklebrasseur
Copy link

Wonder if you should use Regexp instead of Strings

@refack
Copy link
Contributor

refack commented Jun 18, 2018

v8-updates tests are still not running for node-test-commit-v8-linux, even though settings seems to be

I should have verified that node-test-commit-custom-suites actually understands the platform label parameter (and doesn't just run on empty).

https://ci.nodejs.org/job/node-test-commit-custom-suites/20/ seems to be working.
Let's see - https://ci.nodejs.org/job/node-test-commit-v8-linux/1439/

@mmarchini
Copy link
Contributor Author

Worked this time 😄

It's not very intuitive to see the test result (node-test-commit-v8-linux#1439 > nodes=benchmark > node-test-commit-custom-suites#21), but we can improve that in the future if we think it's necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants