Skip to content

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented May 5, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

test, dgram

Description of change

Allow out of order replies in the flaky test-dgram{-upd6,}-send-default-host.js files by sorting the incoming replies after receiving them.

Fixes: #6577

/cc @Trott @santigimeno

@addaleax addaleax added dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests. labels May 5, 2016
client.close();
}
});
}, toSend.length));
Copy link
Member

Choose a reason for hiding this comment

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

Why are you passing toSend.length here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Making sure that the message event is invoked for each message – otherwise the test would accept messages just being dropped.

Copy link
Member

Choose a reason for hiding this comment

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

Apologies, I missed it was an argument to mustCall.

@bnoordhuis
Copy link
Member

LGTM

@cjihrig
Copy link
Contributor

cjihrig commented May 6, 2016

@santigimeno
Copy link
Member

LGTM

1 similar comment
@jasnell
Copy link
Member

jasnell commented May 6, 2016

LGTM

@mcollina
Copy link
Member

mcollina commented May 6, 2016

LGTM

@mcollina
Copy link
Member

mcollina commented May 6, 2016

BTW, can you please sweep through all dgram tests? I think I copied the structure of those from another one, which probably has the same issue.

@addaleax
Copy link
Member Author

addaleax commented May 8, 2016

@mcollina I went through them and there seemed to be no other tests with this problem, just these two.

@mcollina
Copy link
Member

mcollina commented May 8, 2016

@addaleax thx for checking and fixing!

@addaleax addaleax force-pushed the fix-udp-default-host-tests branch from 76d7b00 to 2e66c82 Compare May 10, 2016 05:43
Allow out of order replies in the flaky
`test-dgram{-upd6,}-send-default-host.js` files by sorting
the incoming replies after receiving them.

Fixes: nodejs#6577
PR-URL: nodejs#6607
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@addaleax addaleax force-pushed the fix-udp-default-host-tests branch from 2e66c82 to c5a8bd5 Compare May 10, 2016 05:48
@addaleax addaleax merged this pull request into nodejs:master May 10, 2016
@addaleax addaleax deleted the fix-udp-default-host-tests branch May 10, 2016 05:53
addaleax added a commit to addaleax/node that referenced this pull request May 10, 2016
Allow out of order replies in the flaky
`test-dgram{-upd6,}-send-default-host.js` files by sorting
the incoming replies after receiving them.

PR-URL: nodejs#6607
Fixes: nodejs#6577
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
evanlucas pushed a commit that referenced this pull request May 17, 2016
Allow out of order replies in the flaky
`test-dgram{-upd6,}-send-default-host.js` files by sorting
the incoming replies after receiving them.

PR-URL: #6607
Fixes: #6577
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants