test: cleanup/update test-dgram-empty-packet.js #8896

Goyapa · 2016-10-02T10:39:29Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test

Description of change

Changed some var to const and let
Changed == to === for clarity.

lpinca · 2016-10-02T10:54:36Z

test/parallel/test-dgram-empty-packet.js


 client.bind(0, function() {
+


Nit: I would get rid of this line.

lpinca · 2016-10-02T10:56:16Z

Nit: description says that == has been replaced with includes but I don't see that change in the diff. Did you mean == -> ===?

Goyapa · 2016-10-02T13:29:45Z

@lpinca
I edited "test/parallel/test-dgram-empty-packet.js" locally
got rid of line 16
git add test/parallel/test-dgram-empty-packet.js
git commit --amend
edited the commit message
git push --force-with-lease origin dgram-empty-es6

finally edited comment

Changed == to includes for clarity.
Changed == to === for clarity.

Is this a usual way ore have u any suggestions?

lpinca · 2016-10-02T13:31:33Z

@Goyapa yes, that's fine, thank you!

lpinca

LGTM

mscdex · 2016-10-02T16:05:26Z

test/parallel/test-dgram-empty-packet.js

@@ -28,7 +29,6 @@ client.bind(0, function() {
    callback();
  });

-  const port = this.address().port;


IMHO this is best left here since it's declared right above the only place that it's used.

@mscdex
changed !

imyller

LGTM

mscdex · 2016-10-02T20:46:45Z

LGTM if CI is ok: https://ci.nodejs.org/job/node-test-pull-request/4357/

Trott

Needs some tailing whitespace removed from one line to satisfy the linter. Everything else LGTM.

Trott · 2016-10-02T22:19:09Z

test/parallel/test-dgram-empty-packet.js

-client = dgram.createSocket('udp4');
+const client = dgram.createSocket('udp4');
+
+client.bind(0, function() { 


The linter is failing because of trailing spaces on this line.

make test or (vcbuild test nosign on Windows) would run the linter and would have caught this. If you want to run just the linter without running the whole test suite first, you can run the linter alone with make jslint (or, I think, vcbuild jslint on Windows).

@Trott sorry for that!
git commit --amend
make lint
./configure && make -j8 test

Total errors found: 0
git push --force-with-lease origin dgram-empty-es6

Tested and pushed!

* Changed some `var` to `const` and 'let' * Changed `==` to `===` for clarity.

addaleax

LGTM

cjihrig

LGTM with the requested change.

mscdex · 2016-10-03T14:28:44Z

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

imyller · 2016-10-05T12:51:39Z

Landing:

Approvals (LGTM): 7
No objections
PR has been open for the minimum time of 48 or 72 hours
All of the requested changes have been made
CI tests passed (only known/unrelated failures)

imyller · 2016-10-05T12:56:55Z

Stopped landing. @Goyapa you pushed additional commits?

imyller · 2016-10-05T13:07:56Z

Wow. In very last minute just before landing: from few lines in single file to +1122/−383 lines in 55 files.

@Goyapa merge gone wrong? ;)

I'll take this up for landing prep again once you've cleaned up the source branch.

Goyapa · 2016-10-05T13:29:17Z

@imyller
Did not see your landing attempt! ;-)
Sorry for that!
I pushed the old reference again! :-)

imyller · 2016-10-05T16:16:41Z

@Goyapa No worries. Lets try again 👍

imyller · 2016-10-05T16:16:58Z

Landing:

Approvals (LGTM): 7
No objections
PR has been open for the minimum time of 48 or 72 hours
All of the requested changes have been made
CI tests passed (only known/unrelated failures)

* Changed some `var` to `const` and 'let' * Changed `==` to `===` for clarity. PR-URL: nodejs#8896 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

imyller · 2016-10-05T16:19:41Z

Landed in f68e0d1

Thank you for your effort and contribution, @Goyapa

* Changed some `var` to `const` and 'let' * Changed `==` to `===` for clarity. PR-URL: #8896 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 2, 2016

lpinca reviewed Oct 2, 2016

View reviewed changes

lpinca approved these changes Oct 2, 2016

View reviewed changes

mscdex added the dgram Issues and PRs related to the dgram subsystem / UDP. label Oct 2, 2016

mscdex reviewed Oct 2, 2016

View reviewed changes

imyller approved these changes Oct 2, 2016

View reviewed changes

Trott requested changes Oct 2, 2016

View reviewed changes

test: cleanup/update test-dgram-empty-packet.js

75c402c

* Changed some `var` to `const` and 'let' * Changed `==` to `===` for clarity.

addaleax approved these changes Oct 3, 2016

View reviewed changes

cjihrig approved these changes Oct 3, 2016

View reviewed changes

imyller self-assigned this Oct 5, 2016

imyller closed this Oct 5, 2016

imyller removed their assignment Oct 5, 2016

MylesBorins added the dont-land-on-v4.x label Oct 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: cleanup/update test-dgram-empty-packet.js #8896

test: cleanup/update test-dgram-empty-packet.js #8896

Goyapa commented Oct 2, 2016 •

edited

Loading

lpinca Oct 2, 2016

lpinca commented Oct 2, 2016 •

edited

Loading

Goyapa commented Oct 2, 2016

lpinca commented Oct 2, 2016

lpinca left a comment

mscdex Oct 2, 2016

Goyapa Oct 2, 2016

imyller left a comment

mscdex commented Oct 2, 2016

Trott left a comment

Trott Oct 2, 2016

Trott Oct 2, 2016

Goyapa Oct 3, 2016

addaleax left a comment

cjihrig left a comment

mscdex commented Oct 3, 2016

imyller commented Oct 5, 2016 •

edited

Loading

imyller commented Oct 5, 2016

imyller commented Oct 5, 2016 •

edited

Loading

Goyapa commented Oct 5, 2016 •

edited

Loading

imyller commented Oct 5, 2016

imyller commented Oct 5, 2016

imyller commented Oct 5, 2016

test: cleanup/update test-dgram-empty-packet.js #8896

test: cleanup/update test-dgram-empty-packet.js #8896

Conversation

Goyapa commented Oct 2, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

lpinca Oct 2, 2016

Choose a reason for hiding this comment

lpinca commented Oct 2, 2016 • edited Loading

Goyapa commented Oct 2, 2016

lpinca commented Oct 2, 2016

lpinca left a comment

Choose a reason for hiding this comment

mscdex Oct 2, 2016

Choose a reason for hiding this comment

Goyapa Oct 2, 2016

Choose a reason for hiding this comment

imyller left a comment

Choose a reason for hiding this comment

mscdex commented Oct 2, 2016

Trott left a comment

Choose a reason for hiding this comment

Trott Oct 2, 2016

Choose a reason for hiding this comment

Trott Oct 2, 2016

Choose a reason for hiding this comment

Goyapa Oct 3, 2016

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

mscdex commented Oct 3, 2016

imyller commented Oct 5, 2016 • edited Loading

imyller commented Oct 5, 2016

imyller commented Oct 5, 2016 • edited Loading

Goyapa commented Oct 5, 2016 • edited Loading

imyller commented Oct 5, 2016

imyller commented Oct 5, 2016

imyller commented Oct 5, 2016

Goyapa commented Oct 2, 2016 •

edited

Loading

lpinca commented Oct 2, 2016 •

edited

Loading

imyller commented Oct 5, 2016 •

edited

Loading

imyller commented Oct 5, 2016 •

edited

Loading

Goyapa commented Oct 5, 2016 •

edited

Loading