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.

Sign up for GitHub

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

Jump to bottom

test: move long-running test to sequential #10161

Closed
wants to merge 1 commit into from
Closed

test: move long-running test to sequential #10161

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 7, 2016 •
edited
Loading

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 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 nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 7, 2016
@Trott
Copy link
Member Author

Trott commented Dec 7, 2016

Refs: #9815 (comment)

@Trott
Copy link
Member Author

Trott commented Dec 7, 2016

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

@Trott Trott mentioned this pull request Dec 7, 2016
Closed
3 tasks
@Trott Trott removed the wip Issues and PRs that are still a work in progress. label Dec 7, 2016
@Trott
Copy link
Member Author

Trott commented Dec 7, 2016

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
Copy link
Member Author

Trott commented Dec 7, 2016

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

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

@Trott
Copy link
Member Author

Trott commented Dec 7, 2016

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
Copy link
Member Author

Trott commented Dec 7, 2016 •
edited
Loading

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
Copy link
Contributor

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

@thefourtheye
Copy link
Contributor

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
Copy link
Member Author

Trott commented Dec 7, 2016

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.

@Trott
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
Copy link
Member Author

Trott commented Dec 7, 2016

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

Trott added a commit to Trott/io.js that referenced this pull request Dec 7, 2016
@Trott
test: move long-running test to sequential
cdeb85e 
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 <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@Trott
Copy link
Member Author

Trott commented Dec 7, 2016

Landed in cdeb85e. Thanks.

@Trott Trott closed this Dec 7, 2016
@cjihrig cjihrig mentioned this pull request Dec 7, 2016
Closed
@evanlucas evanlucas mentioned this pull request Dec 7, 2016
Open
addaleax pushed a commit that referenced this pull request Dec 8, 2016
@Trott @addaleax
test: move long-running test to sequential
01509bc 
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 <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@italoacasas italoacasas mentioned this pull request Dec 15, 2016
Merged
@MylesBorins
Copy link
Contributor

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
Copy link
Member Author

Trott commented Dec 20, 2016

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
Copy link
Contributor

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

@Trott
Copy link
Member Author

Trott commented Jan 23, 2017

@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 thefourtheye mentioned this pull request Feb 5, 2017
Closed
3 tasks
@thefourtheye
Copy link
Contributor

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

jasnell pushed a commit that referenced this pull request Mar 3, 2017
@Trott @jasnell
test: move long-running test to sequential
4e90a13 
  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 <santiago.gimeno@gmail.com>
  Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  Reviewed-By: Italo A. Casas <me@italoacasas.com>

Backport-Of: #10161
PR-URL: #11176
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@Trott @MylesBorins
test: move long-running test to sequential
1b85989 
  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 <santiago.gimeno@gmail.com>
  Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  Reviewed-By: Italo A. Casas <me@italoacasas.com>

Backport-Of: #10161
PR-URL: #11176
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott deleted the deflake-smartos branch January 13, 2022 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@santigimeno santigimeno santigimeno approved these changes

@cjihrig cjihrig cjihrig approved these changes

@italoacasas italoacasas italoacasas approved these changes

Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem. smartos Issues and PRs related to the SmartOS platform. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Trott @thefourtheye @MylesBorins @santigimeno @cjihrig @italoacasas @nodejs-github-bot