FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: move long-running test to sequential by Trott · Pull Request #10161 · nodejs/node · GitHub

/ node Public

test: move long-running test to sequential - #10161

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:deflake-smartos
ConversationCommits (1)ChecksFiles changed
Closed

test: move long-running test to sequential#10161
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:deflake-smartos

Conversation

Trott commented Dec 7, 2016 •
edited
Loading

Copy link
Copy Markdown
Member
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 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 commented Dec 7, 2016

Copy link
Copy Markdown
Member Author

Refs: #9815 (comment)

Trott commented Dec 7, 2016

Copy link
Copy Markdown
Member Author

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

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

Trott commented Dec 7, 2016

Copy link
Copy Markdown
Member Author

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 commented Dec 7, 2016

Copy link
Copy Markdown
Member Author

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

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

Trott commented Dec 7, 2016

Copy link
Copy Markdown
Member Author

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 commented Dec 7, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 commented Dec 7, 2016

Copy link
Copy Markdown
Contributor

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

thefourtheye commented Dec 7, 2016

Copy link
Copy Markdown
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 commented Dec 7, 2016

Copy link
Copy Markdown
Member Author

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: 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 commented Dec 7, 2016

Copy link
Copy Markdown
Member Author

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
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 commented Dec 7, 2016

Copy link
Copy Markdown
Member Author

Landed in cdeb85e. Thanks.

Trott closed this Dec 7, 2016
cjihrig mentioned this pull request Dec 7, 2016
Closed
evanlucas mentioned this pull request Dec 7, 2016
Open
addaleax pushed a commit that referenced this pull request Dec 8, 2016
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 mentioned this pull request Dec 15, 2016
Merged

MylesBorins commented Dec 20, 2016

Copy link
Copy Markdown
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 commented Dec 20, 2016

Copy link
Copy Markdown
Member Author

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 commented Jan 23, 2017

Copy link
Copy Markdown
Contributor

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

Trott commented Jan 23, 2017

Copy link
Copy Markdown
Member Author

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

thefourtheye commented Feb 5, 2017

Copy link
Copy Markdown
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
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
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 deleted the deflake-smartos branch January 13, 2022 22:44
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

santigimeno santigimeno approved these changes
cjihrig cjihrig approved these changes
+1 more reviewer
italoacasas italoacasas approved these changes
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL