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.

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

test: shorten path for bogus socket #4478

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 30, 2015

This (hopefully) fixes CI failures for test-net-pipe-connect-errors on Raspberry Pi
devices.

This fixes CI failures for test-net-pipe-connect-errors on Raspberry Pi
devices.
@Trott Trott added the test Issues and PRs related to the tests. label Dec 30, 2015
@Trott
Copy link
Member Author

Trott commented Dec 30, 2015

R=@jbergstroem

@Trott
Copy link
Member Author

Trott commented Dec 30, 2015

@jbergstroem
Copy link
Member

LGTM if ci is green. The 'real' fix here is passing a different temporary path which is in the works.

@Trott
Copy link
Member Author

Trott commented Dec 30, 2015

@jbergstroem CI is green (well, yellow, because one known-flaky test, but you know, green-ish). I'll leave it up to you if you want to let this sit for a bit so others can comment or if you want to expedite a merge on the grounds that CI won't be green until this lands.

@mscdex mscdex added the net Issues and PRs related to the net subsystem. label Dec 30, 2015
@jbergstroem
Copy link
Member

I'd say merge. It fixes current state and won't be in the way for the long term solution.

@jasnell
Copy link
Member

jasnell commented Dec 30, 2015

LGTM

Trott added a commit that referenced this pull request Dec 30, 2015
This fixes CI failures for test-net-pipe-connect-errors on Raspberry Pi
devices.

PR-URL: #4478
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Dec 30, 2015

Landed in bfa925f

@jasnell jasnell closed this Dec 30, 2015
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
This fixes CI failures for test-net-pipe-connect-errors on Raspberry Pi
devices.

PR-URL: nodejs#4478
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

I think that this commit is responsible for failures that we are currently seeing on v5.4.x in CI.

I am a bit flabbergasted as to why this causes failures only on a specific ARM machine, and only on v5.x and not master, but perhaps it is something to do with the way a specific semver minor or semver major change is working.

It seems like the semantics of this fix might be relying on a SemverMajor change that is sitting on master, meaning that it should likely hold off until 6.0 to land.

edit: it appears that the CI on reverting this commit is causing other stuff on ARM to fail (probably what was originally being fixed)

https://ci.nodejs.org/job/node-test-binary-arm/RUN_SUBSET=2,nodes=pi2-raspbian-wheezy/545/tapTestReport/test.tap-81/

@MylesBorins
Copy link
Contributor

It does not appear that reverting the commit helps with anything. In fact it would appear that more things are failing without it. I am at a bit of a loss

@jbergstroem
Copy link
Member

Just a fyi: while tests were running the environment variable node_test_dir wasn't properly set (which it is now).

MylesBorins pushed a commit that referenced this pull request Jan 13, 2016
This fixes CI failures for test-net-pipe-connect-errors on Raspberry Pi
devices.

PR-URL: #4478
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

After discussion with @mhdawson I have opted to back this commit out of the staging branch for the time being. It appears that this commit relies on enhancements found in #3325, specifically the removal of the env var NODE_COMMON_PIPE and the introduction of the NODE_TMP_DIR

Until a decision is made regarding #3325 I'm going to hold off

@jbergstroem
Copy link
Member

I saw this commit more as a temporary patch to fix build issues between #3325 landed and NODE_TMP_DIR properly being in place in jenkins. #3325 supersedes this since it doesn't take NODE_COMMON_PIPE into regard any longer, rather sets it relative to NODE_TMP_DIR.

The problem that #3325 set to solve was that NODE_COMMON_PIPE was being used by multiple test runners if the test suite was running in parallel.

@MylesBorins
Copy link
Contributor

so it would seem to me that an appropriate solution would be to backport #3325 and stop using NODE_COMMON_PIPE everywhere to avoid parallel tests stepping on each others feet

@jbergstroem
Copy link
Member

@thealphanerd Yes, that'd be my suggested solution too. The problem with CI is that we have so many versions of Makefile, different ways of building, releasing and testing node between all branches which makes it hard to have "one true way" of doing something. That's why we can't rely on flags or makefile directives and most often have to do env injections with fallbacks.

Moving back to this PR it should be harmless to merge -- we still have NODE_COMMON_PIPE set on all vm's and will for the foreseeable future.

@MylesBorins
Copy link
Contributor

@mhdawson do you think this is ready to come back in?

@mhdawson
Copy link
Member

@thealphanerd the key issue is if all the dependent pieces landed. It looks like #3325 has landed so its probably ok if that was the only one required.

@MylesBorins
Copy link
Contributor

thanks for chiming in @mhdawson

I will be including this in the next v4.4.0 rc. Please let us know if there is any weirdness on your end due to this

MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
This fixes CI failures for test-net-pipe-connect-errors on Raspberry Pi
devices.

PR-URL: #4478
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
This fixes CI failures for test-net-pipe-connect-errors on Raspberry Pi
devices.

PR-URL: #4478
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
This fixes CI failures for test-net-pipe-connect-errors on Raspberry Pi
devices.

PR-URL: #4478
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
This fixes CI failures for test-net-pipe-connect-errors on Raspberry Pi
devices.

PR-URL: nodejs#4478
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott deleted the shorten-path branch January 13, 2022 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants