Enable --abort-on-timeout for node-test-commit-smartos Jenkins job

[misterdjules]

nodejs/node#11086 introduced a new --abort-on-timeout command line option for tools/test.py that makes tests that timeout abort instead of being terminated.

I'd like to use this flag for test jobs running on SmartOS so that we can get a core file when a test times out and investigate the cause for the test timing out.

When testing the PR mentioned above, I had already added a new test job named "node-test-commit-smartos-abort-on-timeout" that defines a new environment variable TEST_CI_ARGS and sets it to --abort-on-timeout by default.

What do you think of making the same change to the node-test-commit-smartos job?

/cc @nodejs/build

[gibfahn]

Sounds good to me

[misterdjules]

Modified the node-test-commit-smartos job to add a TEST_CI_ARGS parameter with a default value of --abort-on-timeout, and deleted the test job node-test-commit-smartos-abort-on-timeout that had been used to test this change.

[bnoordhuis]

This is breaking the CI for non-master branches, see e.g. https://ci.nodejs.org/job/node-test-commit-smartos/6865/nodes=smartos14-32/console which is a run of v6.x-staging. If this can't be fixed easily and speedily, it should be rolled back for now.

[bnoordhuis]

To save people from clicking through, the error is this:

test.py: error: no such option: --abort-on-timeout

Which makes sense because nodejs/node#11086 hasn't been back-ported yet.

[misterdjules]

My apologies, I disabled the new flag in node-test-commit-smartos for now, and I'll work on a backport asap. Thank you for investigating, and sorry for the trouble!

[bnoordhuis]

Thanks, Julien.

[thefourtheye]

Is it generally okay to backport, given that it hasn't been released in "Current" release stream?

[misterdjules]

Is it generally okay to backport, given that it hasn't been released in "Current" release stream?

I would think so. Is there any indication of the contrary somewhere? FWIW, this change is part of the v7.6.0 release proposal at nodejs/node#11185.

[gibfahn]

Is it generally okay to backport, given that it hasn't been released in "Current" release stream?

I would think so. Is there any indication of the contrary somewhere?

cc/ @MylesBorins @nodejs/lts

[bnoordhuis]

I don't see why not, it's not a user-visible change. It's a change to the test harness, not to node itself.

[thefourtheye]

@bnoordhuis I agree, this specific case is okay. I am wondering, generally, if a commit lands in master, then it is good enough to be backported.

[bnoordhuis]

@thefourtheye Is that a question or a statement? =)

If it is a user-visible change (bug fix, feature, anything that changes the node binary or how it's built) it should bake for some time in Current first.

[thefourtheye]

I was just thinking out loud :D Cool. That makes sense. @bnoordhuis Thanks :-)

[misterdjules]

The changes to support that new tools/test.py command line flag were back ported to v6.x and v4.x branches with nodejs/node#11354 and nodejs/node#11351.

My intention is to re-enable this flag on SmartOS asap. Any objection, or any branch that I missed?

[gibfahn]

@misterdjules sounds good to me. If you get any failures you can always conditionally enable it for v4/v6/v7/master.

[misterdjules]

Enabled again for the node-test-commit-smartos job. Thanks all for the feedback, and sorry for the breakage.

I'll watch builds closely for the next couple of days to make sure it's not breaking anything, and I'll react as quickly as possible if it breaks.