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

Revert "test: don't assume broadcast traffic is unfiltered" #259

Merged
merged 2 commits into from
Jan 8, 2015

Conversation

bnoordhuis
Copy link
Member

This reverts commit 52e600a.

Reverted for:

  • making the test fail with ENETUNREACH on OS X 10.8, and
  • making the test fail with EHOSTDOWN on OS X 10.9 and 10.10 when there
    is no network connectivity, and
  • leaving behind orphan processes that make subsequent tests fail with
    EADDRINUSE errors

R=@chrisdickinson or @rvagg

@rvagg
Copy link
Member

rvagg commented Jan 8, 2015

do you have a CI job for this or shall we submit one?

@rvagg
Copy link
Member

rvagg commented Jan 8, 2015

@rvagg
Copy link
Member

rvagg commented Jan 8, 2015

looks good! a couple of apparently unrelated failures, one for osx and one for windows, so this LGTM

This reverts commit 52e600a.

Reverted for:

* making the test fail with ENETUNREACH on OS X 10.8, and

* making the test fail with EHOSTDOWN on OS X 10.9 and 10.10 when there
  is no network connectivity, and

* leaving behind orphan processes that make subsequent tests fail with
  EADDRINUSE errors

PR-URL: nodejs#259
Reviewed-By: Rod Vagg <rod@vagg.org>
Move parallel/test-dgram-broadcast-multi-process to test/internet.

The test does not play nice with firewalls that restrict broadcast
or multicast traffic, nor can it be rewritten to use only loopback
traffic without running into platform-specific limitations, see also
commits 52e600a and 236533c (TODO: update second one before landing.)

PR-URL: nodejs#259
Reviewed-By: Rod Vagg <rod@vagg.org>
@bnoordhuis bnoordhuis merged commit 7266b75 into nodejs:v1.x Jan 8, 2015
@bnoordhuis bnoordhuis deleted the fix-broadcast-test branch January 8, 2015 12:18
@indutny
Copy link
Member

indutny commented Jan 9, 2015

Haha!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants