test: move long-running test to sequential#10161

Trott · 2016-12-07T05:14:45Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test buffer

Description of change

test-buffer-creation-regression is flaky on some SmartOS hosts in CI,
timing out. Move to sequential to it does not compete with other tests
for resources.

Trott · 2016-12-07T05:15:14Z

Refs: #9815 (comment)

Trott · 2016-12-07T05:17:05Z

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

Trott · 2016-12-07T05:40:00Z

Meh...passed on SmartOs 15 64-bit, but barely didn't time out:

ok 1255 sequential/test-buffer-creation-regression --- duration_ms: 58.896

Will try splitting the test up into three files.

Trott · 2016-12-07T05:51:07Z

OK, split the test into 3. Trying again...

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

Trott · 2016-12-07T06:03:06Z

These results seem more like it:

ok 1255 sequential/test-buffer-creation-regression-1 --- duration_ms: 0.428 ...ok 1256 sequential/test-buffer-creation-regression-2 --- duration_ms: 9.35 ...ok 1257 sequential/test-buffer-creation-regression-3 --- duration_ms: 16.939

Trott · 2016-12-07T06:41:14Z

CI is all green ✅ . I'd like to expedite landing this because this issue is significantly impacting CI results. Reviews, anyone? @nodejs/testing @thefourtheye

thefourtheye · 2016-12-07T06:50:37Z

@Trott All the three tests have more than 90% similar code. Can we refactor and put the common code in one file?

thefourtheye · 2016-12-07T06:50:46Z

Apart from that, I believe the second test itself is enough as regression. Can we modify the original test and remove other two cases?

Trott · 2016-12-07T06:58:53Z

Apart from that, I believe the second test itself is enough as regression. Can we modify the original test and remove other two cases?

For me, at least, running v7.2.0, only the final test case fails. Happy to get rid of the other two and just keep that one, if that works.

test-buffer-creation-regression is flaky on some SmartOS hosts in CI, timing out. Move to sequential so it does not compete with other tests for resources. Reduce three test cases to just the one needed to identify the regression.

Trott · 2016-12-07T07:09:12Z

Reduced to just the one test case needed. CI: https://ci.nodejs.org/job/node-test-pull-request/5286/

test-buffer-creation-regression is flaky on some SmartOS hosts in CI, timing out. Move to sequential so it does not compete with other tests for resources. Reduce three test cases to just the one needed to identify the regression. PR-URL: nodejs#10161 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>

Trott · 2016-12-07T15:37:08Z

Landed in cdeb85e. Thanks.

test-buffer-creation-regression is flaky on some SmartOS hosts in CI, timing out. Move to sequential so it does not compete with other tests for resources. Reduce three test cases to just the one needed to identify the regression. PR-URL: #10161 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>

MylesBorins · 2016-12-20T22:23:07Z

This is failing when backported to v4.x

is that expected behavior?

=== release test-buffer-creation-regression === Path: sequential/test-buffer-creation-regression FATAL ERROR: v8::Uint8Array::New(Local<ArrayBuffer>, size_t, size_t) length exceeds max allowed value Command: out/Release/node /Users/thealphanerd/code/node/v4.x/test/sequential/test-buffer-creation-regression.js

Trott · 2016-12-20T22:56:51Z

test/parallel/test-buffer-creation-regression.js doesn't exist on v4.x-staging, does it? If not then this should be labeled do-not-land-on-v4.x I think...

MylesBorins · 2017-01-23T02:03:43Z

@Trott this is failing on v6.x too. labelled don't land, feel free to backport

Trott · 2017-01-23T04:09:51Z

@Trott this is failing on v6.x too. labelled don't land, feel free to backport

Seems like the original test doesn't exist on v6.x either, so yeah, do not land on v6.x.

thefourtheye · 2017-02-05T14:15:07Z

I backported this in #11176, along with #9815 (which has the test) and added lts watch v6.x.

test-buffer-creation-regression is flaky on some SmartOS hosts in CI, timing out. Move to sequential so it does not compete with other tests for resources. Reduce three test cases to just the one needed to identify the regression. PR-URL: #10161 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Italo A. Casas <[email protected]> Backport-Of: #10161 PR-URL: #11176 Reviewed-By: James M Snell <[email protected]>

Trott added buffer Issues and PRs related to the buffer subsystem. wip Issues and PRs that are still a work in progress. smartos Issues and PRs related to the SmartOS platform. labels Dec 7, 2016

nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 7, 2016

Trott mentioned this pull request Dec 7, 2016
buffer: convert offset & length to int properly #9815
Closed
3 tasks

Trott removed the wip Issues and PRs that are still a work in progress. label Dec 7, 2016

Trott force-pushed the deflake-smartos branch from 4ec0784 to f9c5b61Compare December 7, 2016 05:49

santigimeno approved these changes Dec 7, 2016
View reviewed changes

test: move long-running test to sequential
9a26b69
test-buffer-creation-regression is flaky on some SmartOS hosts in CI, timing out. Move to sequential so it does not compete with other tests for resources. Reduce three test cases to just the one needed to identify the regression.

Trott force-pushed the deflake-smartos branch from f9c5b61 to 9a26b69Compare December 7, 2016 07:02

italoacasas approved these changes Dec 7, 2016
View reviewed changes

cjihrig approved these changes Dec 7, 2016
View reviewed changes

Trott closed this Dec 7, 2016

cjihrig mentioned this pull request Dec 7, 2016
test: test-buffer-creation-regression intermittent failure on smartos #10166
Closed

evanlucas mentioned this pull request Dec 7, 2016
Does not pull all reviewers on some PRs nodejs/node-review#6
Open

italoacasas mentioned this pull request Dec 15, 2016
v7.3.0 proposal #10277
Merged

MylesBorins added lts-watch-v4.x labels Dec 20, 2016

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Jan 23, 2017

thefourtheye mentioned this pull request Feb 5, 2017
Backport 9815 & 10161 to v6.x #11176
Closed
3 tasks

thefourtheye added lts-watch-v6.x and removed dont-land-on-v6.x labels Feb 5, 2017

MylesBorins added dont-land-on-v6.x and removed lts-watch-v6.x labels Mar 7, 2017

Trott deleted the deflake-smartos branch January 13, 2022 22:44

Uh oh!

test: move long-running test to sequential#10161

test: move long-running test to sequential #10161

Uh oh!

Conversation

Trott commented Dec 7, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Trott commented Dec 7, 2016

Uh oh!

Trott commented Dec 7, 2016

Uh oh!

Trott commented Dec 7, 2016

Uh oh!

Trott commented Dec 7, 2016

Uh oh!

Trott commented Dec 7, 2016

Uh oh!

Trott commented Dec 7, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thefourtheye commented Dec 7, 2016

Uh oh!

thefourtheye commented Dec 7, 2016

Uh oh!

Trott commented Dec 7, 2016

Uh oh!

Trott commented Dec 7, 2016

Uh oh!

Trott commented Dec 7, 2016

Uh oh!

MylesBorins commented Dec 20, 2016

Uh oh!

Trott commented Dec 20, 2016

Uh oh!

MylesBorins commented Jan 23, 2017

Uh oh!

Trott commented Jan 23, 2017

Uh oh!

thefourtheye commented Feb 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Trott commented Dec 7, 2016•
edited
Loading

Trott commented Dec 7, 2016•
edited
Loading