test: introduce a broadcast helper #2851

jbergstroem · 2015-09-14T05:34:59Z

Introduce a helper - very similar to common.localhostIPv4 that allows test runner to override a broadcast address. This is required in certain scenarios - like FreeBSD jails - since networking acts a bit different.

Refs: #2472

(I've enabled BROADCAST=$foo on both FreeBSD bots)

/R=@Trott, ?

thefourtheye · 2015-09-14T05:41:29Z

test/common.js

+
+    return broadcastIPv4;
+  }
+})


Missing semicolon

thefourtheye · 2015-09-14T05:43:16Z

test/common.js

+Object.defineProperty(exports, 'broadcastIPv4', {
+
+  get: function() {
+    if (broadcastIPv4 !== null) return broadcastIPv4;


Where is it declared?

jbergstroem · 2015-09-14T05:44:14Z

fixed some linting issues and a missing variable declaration.

Introduce a helper - very similar to `common.localhostIPv4` that allows test runner to override a broadcast address. This is required in certain scenarios - like FreeBSD jails - since networking acts a bit different. Refs: nodejs#2472

Trott · 2015-09-14T05:47:16Z

test/common.js

@@ -107,6 +108,27 @@ Object.defineProperty(exports, 'localhostIPv4', {
  }
 });

+Object.defineProperty(exports, 'broadcastIPv4', {
+


Nit: rm blank line for consistency with the other similar blocks.

Trott · 2015-09-14T05:52:49Z

Nit: There's enough repeated code between the getters for broadcastIPv4 and localhostIPv4 that they should probably both call the same function just passing in an argument telling the function what environment variable to retrieve. (On the other hand, that can be refactored later.)

jbergstroem · 2015-09-14T05:53:31Z

Should be good enough for ci now: https://ci.nodejs.org/job/node-test-pull-request/292/

Trott · 2015-09-14T06:02:56Z

LGTM if CI is happy

jbergstroem · 2015-09-14T06:08:02Z

Lets get the refactor in while at it.

Trott · 2015-09-14T06:23:31Z

This doesn't seem to solve the problem with test-dgram-broadcast-multi-process.js on the 32-bit FreeBSD on the CI. Any ideas? https://ci.nodejs.org/job/node-test-commit-other/629/nodes=freebsd101-32/console https://ci.nodejs.org/job/node-test-commit-other/631/nodes=freebsd101-32/console

jbergstroem · 2015-09-14T06:30:00Z

@Trott I emailed the servers admins a while ago; looks like the broadcast address is set incorrectly.

Trott · 2015-09-14T06:32:07Z

It does seem to fix it for FreeBSD 64-bit, so that's good!

Trott · 2015-09-14T06:34:04Z

And on 32-bit, although it doesn't make the test pass, it is an improvement. Without this change, test output was:

#[PARENT] sent 'First message to send' to 255.255.255.255:12346
#[PARENT] sent 'Second message to send' to 255.255.255.255:12346
#[PARENT] sent 'Third message to send' to 255.255.255.255:12346
#[PARENT] sent 'Fourth message to send' to 255.255.255.255:12346
#[PARENT] sendSocket closed
#[PARENT] Responses were not received within 5000 ms.
#[PARENT] Fail

With this change:

#[PARENT] sent 'First message to send' to 172.16.31.3:12346
#[CHILD] 93497 received 'First message to send' from {"address":"10.20.10.11","family":"IPv4","port":12346,"size":21}
#[PARENT] sent 'Second message to send' to 172.16.31.3:12346
#[CHILD] 93497 received 'Second message to send' from {"address":"10.20.10.11","family":"IPv4","port":12346,"size":22}
#[PARENT] sent 'Third message to send' to 172.16.31.3:12346
#[CHILD] 93497 received 'Third message to send' from {"address":"10.20.10.11","family":"IPv4","port":12346,"size":21}
#[PARENT] sent 'Fourth message to send' to 172.16.31.3:12346
#[CHILD] 93497 received 'Fourth message to send' from {"address":"10.20.10.11","family":"IPv4","port":12346,"size":22}
#[PARENT] sendSocket closed
#[PARENT] 93497 received 4 messages total.
#[PARENT] Responses were not received within 5000 ms.
#[PARENT] Fail

That's several steps in the right direction.

jbergstroem · 2015-09-14T11:00:33Z

@Trott refactor lgtm as well?

Trott · 2015-09-14T16:09:43Z

@jbergstroem Yes, LGTM with the refactor.

thefourtheye · 2015-09-14T17:29:47Z

I prefer writing the function after the variable declaration but I am okay with landing as it is. LGTM

jbergstroem · 2015-09-14T22:24:45Z

I'll see if there's anything we can do in the host machine about broadcast before merging this so we actually "fix" the issue. Otherwise we might have to work around it in our CI.

jbergstroem · 2015-09-14T22:42:37Z

Host rebooted, should've fixed the broadcast. I think we have a winner: https://ci.nodejs.org/job/node-test-pull-request/300/

@thefourtheye I'll move the function below.

@Trott can you confirm that it closes #2472 ?

Trott · 2015-09-14T23:08:29Z

https://ci.nodejs.org/job/node-test-commit-other/647/

It's actually worse now. Now both 64-bit and 32-bit FreeBSD are failing and the test-dgram-multicast-multi-process.js is failing on them as well as test-dgram-broadcast-multi-process.js.

On the upside, they are getting a specific broadcast address that isn't 255.255.255.255.

jbergstroem · 2015-09-15T00:00:52Z

That's unfortunate. This PR should be pretty much ok to land since it at least gives us control. I'll avoid adding "fixed" then.

Trott · 2015-09-15T01:03:19Z

Yeah, actually, even though the tests failed, they're failing further along than before, so it's progress!

Trott · 2015-09-15T01:04:17Z

LGTM

jbergstroem · 2015-10-07T08:41:44Z

Just wanted to update. We're still stuck with figuring out why broadcasting from jails aren't working as intended. This PR will work just fine but the test will still fail [for other reasons]. Not sure we should merge this just yet since it might be avoided by finding another solution in CI-land.

jbergstroem · 2015-10-26T00:26:29Z

Thinking we might skip this -- ref nodejs/build#228.

jasnell · 2015-11-14T05:12:21Z

@jbergstroem ... where are we at on this?

jbergstroem · 2015-11-14T21:32:32Z

We still have the issue but are migrating away from jails in ci. I'm inclined to close and reopen if some other freebsd user can help debug (I didn't have access to host).

mscdex added the test Issues and PRs related to the tests. label Sep 14, 2015

thefourtheye reviewed Sep 14, 2015
View reviewed changes

test/common.js

return broadcastIPv4;

}

})

Copy link

Contributor

thefourtheye Sep 14, 2015

Choose a reason for hiding this comment

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

Missing semicolon

jbergstroem force-pushed the fix/bug-2472 branch from 0b547fd to 245799c Compare September 14, 2015 05:42

thefourtheye reviewed Sep 14, 2015
View reviewed changes

jbergstroem force-pushed the fix/bug-2472 branch from 245799c to 1feb451 Compare September 14, 2015 05:43

test: introduce a broadcast helper

48f0a61

Introduce a helper - very similar to `common.localhostIPv4` that allows test runner to override a broadcast address. This is required in certain scenarios - like FreeBSD jails - since networking acts a bit different. Refs: nodejs#2472

jbergstroem force-pushed the fix/bug-2472 branch from 1feb451 to 48f0a61 Compare September 14, 2015 05:46

Trott reviewed Sep 14, 2015
View reviewed changes

feature: refactor getting environment variables

b725f03

fix: move function below variable declaration

c230032

jbergstroem mentioned this pull request Sep 16, 2015

freebsd: assorted updates nodejs/build#195

Merged

jbergstroem closed this Nov 14, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: introduce a broadcast helper #2851

test: introduce a broadcast helper #2851

jbergstroem commented Sep 14, 2015

thefourtheye Sep 14, 2015

thefourtheye Sep 14, 2015

jbergstroem commented Sep 14, 2015

Trott Sep 14, 2015

Trott commented Sep 14, 2015

jbergstroem commented Sep 14, 2015

Trott commented Sep 14, 2015

jbergstroem commented Sep 14, 2015

Trott commented Sep 14, 2015

jbergstroem commented Sep 14, 2015

Trott commented Sep 14, 2015

Trott commented Sep 14, 2015

jbergstroem commented Sep 14, 2015

Trott commented Sep 14, 2015

thefourtheye commented Sep 14, 2015

jbergstroem commented Sep 14, 2015

jbergstroem commented Sep 14, 2015

Trott commented Sep 14, 2015

jbergstroem commented Sep 15, 2015

Trott commented Sep 15, 2015

Trott commented Sep 15, 2015

jbergstroem commented Oct 7, 2015

jbergstroem commented Oct 26, 2015

jasnell commented Nov 14, 2015

jbergstroem commented Nov 14, 2015

test: introduce a broadcast helper #2851

test: introduce a broadcast helper #2851

Conversation

jbergstroem commented Sep 14, 2015

thefourtheye Sep 14, 2015

Choose a reason for hiding this comment

thefourtheye Sep 14, 2015

Choose a reason for hiding this comment

jbergstroem commented Sep 14, 2015

Trott Sep 14, 2015

Choose a reason for hiding this comment

Trott commented Sep 14, 2015

jbergstroem commented Sep 14, 2015

Trott commented Sep 14, 2015

jbergstroem commented Sep 14, 2015

Trott commented Sep 14, 2015

jbergstroem commented Sep 14, 2015

Trott commented Sep 14, 2015

Trott commented Sep 14, 2015

jbergstroem commented Sep 14, 2015

Trott commented Sep 14, 2015

thefourtheye commented Sep 14, 2015

jbergstroem commented Sep 14, 2015

jbergstroem commented Sep 14, 2015

Trott commented Sep 14, 2015

jbergstroem commented Sep 15, 2015

Trott commented Sep 15, 2015

Trott commented Sep 15, 2015

jbergstroem commented Oct 7, 2015

jbergstroem commented Oct 26, 2015

jasnell commented Nov 14, 2015

jbergstroem commented Nov 14, 2015