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

Removes call to net.Socket.resume #8679

Closed
wants to merge 1 commit into from
Closed

Removes call to net.Socket.resume #8679

wants to merge 1 commit into from

Conversation

ALJCepeda
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Description of change

resume is not necessary since pause is never called
Ref: #4640

@ALJCepeda
Removes call to net.Socket.resume
f376f67 
`resume` is not necessary since `pause` is never called
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 21, 2016
@mscdex mscdex added the child_process Issues and PRs related to the child_process subsystem. label Sep 21, 2016
@Trott
Copy link
Member

Trott commented Sep 21, 2016

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

@ALJCepeda
Copy link
Contributor Author

@Trott I'm having trouble understanding why the test is failing. I see that it has 3 failures for pi1-raspbian-wheezy:

  1. Makefile:207: recipe for target 'test-ci-js' failed
  2. warning: failed to remove out/Release/.nfs00000000004845470000018a
  3. Makefile:207: recipe for target 'test-ci-js' failed

I'm confused because I don't know how this applies to the changes I've made or even how to address them.

On a related note, Is it possible for me to run these CI tests before I make another pull request?

@gibfahn
Copy link
Member

gibfahn commented Sep 21, 2016

@ALJCepeda the three failures on the Raspberry Pis look unrelated, we're currently working on fixing infrastructure issues and general flakyness on the Pis.

Running make j8 test on your machine will run the same set of testsuites as we run on the CI, the CI run should catch any platform specific issues (it's not normally feasible to expect people to test on all supported platforms!) If make -j8 test passed on your machine, then that's all we'd reasonably expect you to test.

@gibfahn
Copy link
Member

gibfahn commented Sep 21, 2016

Incidentally, if you look at https://ci.nodejs.org/job/node-test-pull-request/ you can see that the last ~30 runs of that job have failed, suggesting an infrastructure issue.

I'll try the CI again, but I suspect we'll have to wait until the underlying issues are fixed.

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

@ALJCepeda
Copy link
Contributor Author

@gibfahn I must have looked at the wrong history because I saw other builds were passing and assumed the fail had to be specific to my changes. Now I know where to check, Thank you!

@gibfahn
Copy link
Member

gibfahn commented Sep 21, 2016

New CI has the same problem, but as the failures were only on ARM, and the test that was changed is actually still passing on ARM (link), I think we can safely say that the CI passed.

However it might be worth doing a stress test just to make sure there aren't any intermittent failures. Thoughts @Trott ?

jasnell
jasnell approved these changes Sep 21, 2016
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gibfahn
Copy link
Member

gibfahn commented Sep 21, 2016

LGTM with or without stress test

@Trott
Copy link
Member

Trott commented Sep 21, 2016

Stress test on all platforms (running the test 100 times): https://ci.nodejs.org/job/node-stress-single-test/930/

imyller
imyller approved these changes Sep 21, 2016
View reviewed changes
Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@imyller imyller self-assigned this Sep 22, 2016
@imyller
Copy link
Member

imyller commented Sep 22, 2016

New CI (with V8 5.4 now in master): https://ci.nodejs.org/job/node-test-pull-request/4230/

@imyller
Copy link
Member

imyller commented Sep 23, 2016 •
edited
Loading

I'll start landing this:

  • Three LGTMs
  • No objections
  • CI passed (unrelated CI failures)
  • Stress test passed

Update still holding for ~3 more hours for full 48 hour PR window

@gibfahn
Copy link
Member

gibfahn commented Sep 23, 2016

Yet another CI (build failures): https://ci.nodejs.org/job/node-test-pull-request/4235/

This should be good to land anyway (previous CI runs all look good except for infra issues)

@imyller
Copy link
Member

imyller commented Sep 23, 2016

CI still has failures, but looks like they are either known infra issues or issues having some other reason (mostly V8 related failures)

@imyller
Copy link
Member

imyller commented Sep 23, 2016

I'll start landing this:

  • Three LGTMs
  • No objections
  • CI passed (unrelated CI failures)
  • Stress test passed

imyller pushed a commit to imyller/node that referenced this pull request Sep 23, 2016
@ALJCepeda @imyller
test: remove call to net.Socket.resume()
d9a6d4a 
In test parallel/child-process-fork-net2 `net.Socket.resume()` is not necessary
since `net.Socket.pause()` is never called.

PR-URL: nodejs#8679
Refs: nodejs#4640
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@imyller
Copy link
Member

imyller commented Sep 23, 2016

landed in d9a6d4a

Thank you for your effort and contribution, @ALJCepeda

@imyller imyller closed this Sep 23, 2016
@imyller imyller removed their assignment Sep 23, 2016
jasnell pushed a commit that referenced this pull request Sep 29, 2016
@ALJCepeda @jasnell
test: remove call to net.Socket.resume()
40f1df2 
In test parallel/child-process-fork-net2 `net.Socket.resume()` is not necessary
since `net.Socket.pause()` is never called.

PR-URL: #8679
Refs: #4640
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@MylesBorins
Copy link
Contributor

thoughts re lts?

@jasnell
Copy link
Member

jasnell commented Oct 7, 2016

should be fine for v4 I think.

Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@ALJCepeda @Fishrock123
test: remove call to net.Socket.resume()
b62735a 
In test parallel/child-process-fork-net2 `net.Socket.resume()` is not necessary
since `net.Socket.pause()` is never called.

PR-URL: #8679
Refs: #4640
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
MylesBorins pushed a commit that referenced this pull request Nov 18, 2016
@ALJCepeda
test: remove call to net.Socket.resume()
03f7031 
In test parallel/child-process-fork-net2 `net.Socket.resume()` is not necessary
since `net.Socket.pause()` is never called.

PR-URL: #8679
Refs: #4640
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@MylesBorins MylesBorins mentioned this pull request Nov 22, 2016
Merged
@snyk-bot snyk-bot mentioned this pull request Jul 17, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@imyller imyller imyller approved these changes

@jasnell jasnell jasnell approved these changes

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

Successfully merging this pull request may close these issues.
8 participants
@ALJCepeda @Trott @gibfahn @imyller @MylesBorins @jasnell @mscdex @nodejs-github-bot