Jenkins job node-test-pull-request is replacing iojs+any-pr+multi

[orangemocha]

To all @nodejs/collaborators:

As part of the CI convergence effort, we are rolling out a new Jenkins job to replace iojs+any-pr+multi. The next time you need to test a PR in Jenkins, please use node-test-pull-request instead of iojs+any-pr+multi.

node-test-pull-request is very similar to iojs+any-pr+multi, with the following improvements:

1. A slightly simplified interface. You just need to enter the org & repo name target (a.k.a. base) of the PR, and the PR ID. Note that iojs+any-pr+multi took the org and repo name of the source of the PR. The branches are inferred automatically. You can leave NODES_SUBSET to auto.
2. It works with all current repos and branches. Specifically: joyent/node (v0.10, v0.12, master), nodejs/io.js (master, next). It does not work with nodejs/node yet.
3. The PR is automatically rebased on the target branch before testing. Since we are striving to move logic away from Jenkins and into scripts in the repos, rebasing ensures that the job will work with older PRs. If there is a merge conflict the job will fail and you’ll need to rebase your changes, update the PR, and run the job again. This is probably a good thing, since you would have to address the merge conflicts sooner or later anyway.

iojs+any-pr+multi will be kept around as backup during the initial transition, and beyond that so that we can still access the logs of past runs. In a couple of weeks, barring any unforeseen issues, we are going to rename it to something like DEPRECATED-iojs+any-pr+multi.

For any questions or issues with node-test-pull-request please mention @nodejs/build

[brendanashworth]

@orangemocha is there a way to specify a branch, like next for a PR to master?

[Fishrock123]

So uh, how do we test branches rather than PRs?

The PR is automatically rebased on the target branch before testing. Since we are striving to move logic away from Jenkins and into scripts in the repos, rebasing ensures that the job will work with older PRs. If there is a merge conflict the job will fail and you’ll need to rebase your changes, update the PR, and run the job again. This is probably a good thing, since you would have to address the merge conflicts sooner or later anyway.

I strongly disagree. People may not have time to rebase, or they may not know how. I think it is very important that we are able to run CI tests even in those cases without having to make new PRs.

DEPRECATED-iojs+any-pr+multi

Should be changed to "node-test-git-branch", for reasons noted above.

It works with all current repos and branches. Specifically: joyent/node (v0.10, v0.12, master), nodejs/io.js (master, next). It does not work with nodejs/node yet.

Why is that? iojs+any-pr+multi worked on any repo. (This is important to maintain in a CI job.)

[orangemocha]

@orangemocha is there a way to specify a branch, like next for a PR to master?

@brendanashworth , assuming you mean for the rebasing. You can use the underlying job node-test-commit, which is a bit more low-level and exposes additional features.

Let's say you wanted to test #2085 and rebase it against next instead of master. You would pass the parameters as follows to node-test-commit:

• GITHUB_ORG: nodejs
• REPO_NAME: io.js
• GIT_REMOTE_REF: refs/pull/2085/head
• REBASE_ONTO: origin/next

.. leaving POST_REBASE_SHA1_CHECK, NODES_SUBSET, IGNORE_FLAKY_TESTS to their default values.

Makes sense to you? I tried to make node-test-pull-request simpler to use for the common case, but if specifying the branch to rebase onto is a common scenario and if node-test-commit is a bit too obscure, we could add back the TARGET_BRANCH parameter to node-test-pull-request, with a default of auto.

[orangemocha]

Why is that? iojs+any-pr+multi worked on any repo.

@Fishrock123 , not exactly. The problem is that all these Jenkins jobs rely on scripts/logic in the repo, like the test-ci and run-ci makefile rules. If an older PR source branch does not contain these rules or is not up to date with the way that Jenkins expects them, the job won't work. Which is why we went for the rebasing approach.

I think it is very important that we are able to run CI tests even in those cases without having to make new PRs.

The underlying node-test-commit allows doing this. For testing #2085 without rebasing, you could specify:

• GITHUB_ORG: nodejs
• REPO_NAME: io.js
• GIT_REMOTE_REF: refs/pull/2085/head
• REBASE_ONTO:

..or even (more like iojs+any-pr+multi):

• GITHUB_ORG: Fishrock123
• REPO_NAME: io.js
• GIT_REMOTE_REF: refs/heads/combined-tsc
• REBASE_ONTO:

DEPRECATED-iojs+any-pr+multi

Should be changed to "node-test-git-branch", for reasons noted above.

We could definitely add an entry point 'node-test-branch' to invoke node-test-commit with the right arguments as shown above. It wouldn't be the old iojs+any-pr+multi renamed though. I should have mentioned in the original announcement that node-test-pull-request is just an entry point to node-test-commit, with a PR-friendly set of parameters, and that node-test-commit supports richer functionality, including that needed by the automated merge job, node-accept-pull-request (yet to become available for general consumption).
Or, we could make the rebase optional on node-test-pull-request by adding a checkbox. In any case, because of the dependencies of Jenkins on repo scripts, I feel that the recommended workflow would have to be to test with rebasing first, and only if that fails because of merge conflicts and you don't want to update the PR, to test without rebasing.

Thank you for your feedback! Please keep it coming...

[bnoordhuis]

FWIW, I think rebase-before-build is an excellent idea. It's going to happen anyway when the PR lands and we've had issues in the past where an old PR passed CI but broke after landing.

[silverwind]

What I like to see is a single page with all failures listed, across platforms. On the old CI, It's kind of time consuming having to click through all the layers to finally reach the pages listing failures.

[Fishrock123]

I think most of my concerns are alleviated by the underlying use of node-test-commit.

It's been some days notice so I'll close this. Thanks for all the work @orangemocha :)