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

test: improve assert message for debugging #6581

Closed
wants to merge 1 commit into from
Closed

test: improve assert message for debugging #6581

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented May 4, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

Previously, the assertion changed in this commit would print the same
message no matter which of the four test cases failed. The assert has
been changed so that it will indicate which test case failed.

Refs: #6577

@Trott
test: improve assert message for debugging
2268660 
Previously, the assertion changed in this commit would print the same
message no matter which of the four test cases failed. The assert has
been changed so that it will indicate which test case failed.

Refs: nodejs#6577
@Trott Trott added the test Issues and PRs related to the tests. label May 4, 2016
@Trott Trott mentioned this pull request May 4, 2016
Closed
@addaleax
Copy link
Member

addaleax commented May 4, 2016

LGTM

2 similar comments
@santigimeno
Copy link
Member

LGTM

@jasnell
Copy link
Member

jasnell commented May 4, 2016

LGTM

@mscdex mscdex added the dgram Issues and PRs related to the dgram subsystem / UDP. label May 4, 2016
@mcollina
Copy link
Member

mcollina commented May 5, 2016

LGTM

@mcollina
Copy link
Member

mcollina commented May 5, 2016 •
edited
Loading

@Trott while you are at it, you should update the allocation of the buffers to the new method, like what was done for the ipv4 equivalent: https://github.com/nodejs/node/blob/master/test/parallel/test-dgram-send-default-host.js#L9-L12

@JacksonTian
Copy link
Contributor

LGTM

@mcollina
Copy link
Member

mcollina commented May 5, 2016

I think the commit message needs to be a little more explicative
Il giorno gio 5 mag 2016 alle 11:44 Jackson Tian notifications@github.com
ha scritto:

LGTM

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#6581 (comment)

@cjihrig
Copy link
Contributor

cjihrig commented May 5, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented May 6, 2016

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

@addaleax
Copy link
Member

addaleax commented May 6, 2016

If @Trott is okay with that, this should probably be not be landed in favour of #6607. This PR served its purpose well enough. :)

@jasnell
Copy link
Member

jasnell commented May 6, 2016

SGTM

@Trott
Copy link
Member Author

Trott commented May 6, 2016

If @Trott is okay with that, this should probably be not be landed in favour of #6607.

I agree. Closing this one.

@Trott Trott closed this May 6, 2016
@Trott Trott deleted the msg branch January 13, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
dgram Issues and PRs related to the dgram subsystem / UDP. 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
@Trott @addaleax @santigimeno @jasnell @mcollina @JacksonTian @cjihrig @mscdex