Skip to content
This repository has been archived by the owner on Aug 7, 2023. It is now read-only.

Code And Learn at NodeFest 2016 #58

Closed
yosuke-furukawa opened this issue Sep 28, 2016 · 45 comments
Closed

Code And Learn at NodeFest 2016 #58

yosuke-furukawa opened this issue Sep 28, 2016 · 45 comments

Comments

@yosuke-furukawa
Copy link
Member

Hi folks.
NodeFest is the largest Node.js conference at Japan.

I am an organizer of the conference, I would like to open code-and-learn-jp on NodeFest.
Japan has 1 collaborator (me) and 1 core team member (shigeki) and some great nodeschool staffs (martin, ledsun, watilde, tako-black, etc). We would like to support contribution to nodejs/core.

And I would like to list up the contribution point of Japanese node beginners.
I guess we will receive some PRs on 12th November.
We will follow English communication and some other commit techniques.

@yosuke-furukawa
Copy link
Member Author

Contributing Point:

doc typo:

  • doc/modules.md: line 132 lookups, it would be better looks up.
  • doc/cluster.md: line 147, 499 has eg., it would be better e.g..
  • doc/repl: line 6 has includable, it would be better includible. http://wikidiff.com/includable/includible
  • doc/dns.md: line 263 e.g., => e.g.
  • doc/http.md: line 553/856 e.g., => e.g.
  • doc/tls.md: line 762 836 1026 e.g., => e.g.

doc improvement:

  • doc/url.md does not have WHATWG-URL standard link and the description.

@Trott
Copy link
Member

Trott commented Nov 6, 2016

If you need more issues, one thing you can do is search through the test files for assert.equal(). In almost all cases, the test could be improved by using assert.strictEqual() instead. The one exception I can think of might be test-assert.js where use of assert.equa() may be necessary.

@yosuke-furukawa
Copy link
Member Author

Contributing Point:

test:

  • test/parallel/test-http-status-reason-invalid-chars.js: use port 0 instead of common.PORT,
const server = http.createServer((req, res) => {
  if (req.url === '/explicit') {
    explicit(req, res);
  } else {
    implicit(req, res);
  }
}).listen(common.PORT, common.mustCall(() => { // should use 0
  const url = `http://localhost:${common.PORT}`; // should use server.address().port
  let left = 2;
  const check = common.mustCall((res) => {
    left--;
    assert.notEqual(res.headers['content-type'], 'text/html');
    assert.notEqual(res.headers['content-type'], 'gotcha');
    if (left === 0) server.close();
  }, 2);
  http.get(`${url}/explicit`, check).end();
  http.get(`${url}/implicit`, check).end();
}));

@yosuke-furukawa
Copy link
Member Author

@Trott thank you!!! sound so good. I am currently searching contribution point on code-and-learn on nodefest.

@Trott
Copy link
Member

Trott commented Nov 6, 2016

Here's another one: There are a number of instances of setTimeout() where the function is called without a duration (the second argument). Node.js will treat that the same as calling it with a duration of 1. In many of these cases, it might be better to use setImmediate().

Here are some of those cases. I have not reviewed them to verify that setImmediate() is better, but that might be an appropriate exercise for a first time committer.

  • test/sequential/test-next-tick-error-spin.js line 43
  • test/parallel/test-tls-hello-parser-failure.js line 30
  • test/parallel/test-stream2-writable.js line 331

@yosuke-furukawa
Copy link
Member Author

test: use port 0 instead of common.PORT

  • test/parallel/test-cluster-net-send.js
  server.listen(common.PORT, function() { // use 0 port
    socket = net.connect(common.PORT, '127.0.0.1', socketConnected); // use server.address().port
  });
  • test/parallel/test-tls-connect-address-family.js
  • test/parallel/test-net-socket-destroy-twice.js

@Trott
Copy link
Member

Trott commented Nov 6, 2016

Japan has 1 collaborator (me) and 1 core team member (shigeki)

@ronkorving is a collaborator and lives in Tokyo.

@yosuke-furukawa
Copy link
Member Author

@Trott ah, that's so good.
I will list these points.

@ronkorving
Copy link

@Trott @yosuke-furukawa Indeed :) I'm not too active in the Japanese Node community, but wouldn't mind trying harder :) My Japanese is not yet 100% what I would like it to be. In any case, I signed up for Sunday (visitor) and wouldn't mind helping with the organization next year either if that's appreciated. I'll see you on Sunday. 楽しみにしてます ^^

@jasnell
Copy link
Member

jasnell commented Nov 7, 2016

@yosuke-furukawa I'm definitely looking forward to being there and will help out in whatever way that I can. I just want to confirm that the Code and Learn is scheduled for Saturday afternoon, correct? I want to take a little bit of time Saturday morning to sight see a little bit and I want to make sure I plan things so I won't be late to the Code and Learn :-)

@shigeki
Copy link

shigeki commented Nov 8, 2016

I'm going to join it to help participants and take care of what tests and docs can be good for the first contributions. But actually, the class is just one and half hour so I think it is too short for novices.

Instead of submitting a new PR to nodejs repo, I made an alternative repo of node to give such users the first experiences of contributions in https://github.com/shigeki/node_testpr/ . It can be a training for them if they submit a fake PR such as shigeki/node_testpr#1 .

Users can choice which repo they want to work on during the course.

@shigeki
Copy link

shigeki commented Nov 8, 2016

I've just submitted some issues of test failures to be reserved for this course in nodejs/node#9509, nodejs/node#9510 and nodejs/node#9511

@yosuke-furukawa
Copy link
Member Author

@jasnell

I just want to confirm that the Code and Learn is scheduled for Saturday afternoon, correct?

Yes!!! Code And Learn will begin 15:45 on Saturday, and the end time is 17:15.

And I really would like you to attend NodeDiscussion on Saturday morning, almost 90 min (11:00-12:30). This discussion is unconference style. We would like to discuss about Node.js good point/bad point/wishlist. I would like share our opinions to node core committers.

If you don't have any time on Saturday morning, it is OK, but if you have time, please come!!!!!

@yosuke-furukawa
Copy link
Member Author

@ronkorving Ya!!! please help us !!!! You can come our venue and speak to our receptionist as mentor. you can join :)

@yosuke-furukawa
Copy link
Member Author

@shigeki awesome!!! I will wrap up these contribution point on this Thursday.

@Trott
Copy link
Member

Trott commented Nov 10, 2016

test/sequential/test-next-tick-error-spin.js line 43

You can remove that one from the list as I'm in the process of fixing it while refactoring a bunch of other things in the test. Sorry about that.

You can replace it, though with these tests which all have at least one instance of setTimeout() called without a second argument, and thus are candidates for possibly using setImmediate() instead:

  • test/parallel/test-stream2-readable-wrap.js line 61
  • test/parallel/test-stream2-readable-non-empty-end.js lines 19 and 34
  • test/parallel/test-stream2-push.js line 116
  • test/parallel/test-stream2-large-read-stall.js line 49
  • test/parallel/test-stream-unshift-read-race.js line 44
  • test/parallel/test-stream-unshift-empty-chunk.js line 16
  • test/parallel/test-stream-transform-objectmode-falsey-value.js line 33

@ronkorving
Copy link

@yosuke-furukawa Sounds good, I'll try to make it! :) Basically just show up before 3:45?

@yosuke-furukawa
Copy link
Member Author

@Trott thank you soooooooooooooooo much!!!!!!!!!!!!!!!!!!!!!!

@yosuke-furukawa
Copy link
Member Author

@ronkorving Yes! we will start at 15:45 on dots Shibuya, tokyo. https://eventdots.jp/space

@Fishrock123
Copy link

https://coverage.nodejs.org/coverage-fb05e31466ac0bad/root/internal/process/stdio.js.html

  1. A pseudo-tty test that manually fires 'SIGWINCH' and makes sure none of the properties set by _refreshSize have been changed.
  2. Add the stderr.destroy/stderr.destroySoon error checking to the test which already checks that for stdout.

@yosuke-furukawa
Copy link
Member Author

@Fishrock123
Thank you sooooooooo much !!!!! This coverage information is really helpful for us.

@yosuke-furukawa
Copy link
Member Author

yosuke-furukawa commented Nov 12, 2016

All contribution point:
docs:

  • doc/modules.md: line 132 lookups, it would be better looks up.
  • doc/cluster.md: line 147, 499 has eg., it would be better e.g..
  • doc/repl: line 6 has includable, it would be better includible. http://wikidiff.com/includable/includible
  • doc/dns.md: line 263 e.g., => e.g.
  • doc/http.md: line 553/856 e.g., => e.g.
  • doc/tls.md: line 762 836 1026 e.g., => e.g.

doc improvement:

  • doc/url.md does not have WHATWG-URL standard link and the description.

test assertion improvement:

  • assert.equal would be better to use assert.strictEqual

test failure with build option :

test timing API improvement:
Use setImmediate instead of setTimeout().

  • test/parallel/test-tls-hello-parser-failure.js line 30
  • test/parallel/test-stream2-writable.js line 331
  • test/parallel/test-stream2-readable-wrap.js line 61
  • test/parallel/test-stream2-readable-non-empty-end.js lines 19 and 34
  • test/parallel/test-stream2-push.js line 116
  • test/parallel/test-stream2-large-read-stall.js line 49
  • test/parallel/test-stream-unshift-read-race.js line 44
  • test/parallel/test-stream-unshift-empty-chunk.js line 16
  • test/parallel/test-stream-transform-objectmode-falsey-value.js line 33

test use port:0 instead of common.PORT :

  • test/parallel/test-http-status-reason-invalid-chars.js: use port 0 instead of common.PORT,
  • test/parallel/test-cluster-net-send.js
  • test/parallel/test-tls-connect-address-family.js
  • test/parallel/test-net-socket-destroy-twice.js

coverage improvement:

  • A pseudo-tty test that manually fires 'SIGWINCH' and makes sure none of the properties set by _refreshSize have been changed.
  • Add the stderr.destroy/stderr.destroySoon error checking to the test which already checks that for stdout.

https://coverage.nodejs.org/coverage-fb05e31466ac0bad/root/internal/process/stdio.js.html

@Trott
Copy link
Member

Trott commented Nov 12, 2016

assert.deepEqual would be better to use assert.deepStrictEqual

We have a lint rule for that so you shouldn't find any examples of deepEqual that haven't been explicitly excused as necessary exceptions (e.g., in test-assert.js).

@yosuke-furukawa
Copy link
Member Author

@Trott OK, I updated.

@dorako321
Copy link

dorako321 commented Nov 12, 2016

i will try to fix this doc/dns.md: line 263 e.g., => e.g.

@akito0107
Copy link

I will try to fix test/parallel/test-cluster-net-send.js !

MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 20, 2016
This commit adds the test case of PassThrough.
This test case checks that PassThrough can
construct without new operator.

This is a part of Code And Learn at NodeFest 2016

Fixes: nodejs/code-and-learn#58
PR-URL: #9581
Reviewed-By: Yosuke Furukawa <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 20, 2016
use setImmediate() insted of setTimeout()
 in test of stream2 push.
The test is in test/parallel/test-stream2-push.js

Fixes: nodejs/code-and-learn#58
PR-URL: #9583
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 21, 2016
This commit improves the test cases in
test-stream2-objects.js by using assert.strictEqual
instead of assert.equal.

This is a part of Code And Learn at NodeFest 2016

Fixes: nodejs/code-and-learn#58
PR-URL: #9565
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 21, 2016
This is a part of Code And Learn at NodeFest 2016 Challenge

Fixes: nodejs/code-and-learn#58
PR-URL: #9578
Reviewed-By: Yosuke Furukawa <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 21, 2016
This commit adds the test case of PassThrough.
This test case checks that PassThrough can
construct without new operator.

This is a part of Code And Learn at NodeFest 2016

Fixes: nodejs/code-and-learn#58
PR-URL: #9581
Reviewed-By: Yosuke Furukawa <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 21, 2016
use setImmediate() insted of setTimeout()
 in test of stream2 push.
The test is in test/parallel/test-stream2-push.js

Fixes: nodejs/code-and-learn#58
PR-URL: #9583
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 21, 2016
fix e.g., to e.g. in doc/http.md

Fixes: nodejs/code-and-learn#58
PR-URL: #9564
Reviewed-By: Yosuke Furukawa <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 21, 2016
fix doc/tls.md: line 762 836 1026 e.g., => e.g.

Fixes: nodejs/code-and-learn#58
PR-URL: #9566
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 21, 2016
Fixes: nodejs/code-and-learn#58
PR-URL: #9568
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 21, 2016
Fixes: nodejs/code-and-learn#58
PR-URL: #9563
Reviewed-By: Yosuke Furukawa <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 21, 2016
fix doc/api/repl.md line 6 "includable" => "includible"

Fixes: nodejs/code-and-learn#58
PR-URL: #9582
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 21, 2016
fix e.g., to e.g. in doc/http.md

Fixes: nodejs/code-and-learn#58
PR-URL: #9564
Reviewed-By: Yosuke Furukawa <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 21, 2016
Fixes: nodejs/code-and-learn#58
PR-URL: #9568
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 21, 2016
Fixes: nodejs/code-and-learn#58
PR-URL: #9563
Reviewed-By: Yosuke Furukawa <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 21, 2016
This commit improves the test cases in
test-stream2-objects.js by using assert.strictEqual
instead of assert.equal.

This is a part of Code And Learn at NodeFest 2016

Fixes: nodejs/code-and-learn#58
PR-URL: #9565
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 21, 2016
This is a part of Code And Learn at NodeFest 2016 Challenge

Fixes: nodejs/code-and-learn#58
PR-URL: #9578
Reviewed-By: Yosuke Furukawa <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 21, 2016
This commit adds the test case of PassThrough.
This test case checks that PassThrough can
construct without new operator.

This is a part of Code And Learn at NodeFest 2016

Fixes: nodejs/code-and-learn#58
PR-URL: #9581
Reviewed-By: Yosuke Furukawa <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Dec 21, 2016
use setImmediate() insted of setTimeout()
 in test of stream2 push.
The test is in test/parallel/test-stream2-push.js

Fixes: nodejs/code-and-learn#58
PR-URL: #9583
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
sreepurnajasti added a commit to sreepurnajasti/node that referenced this issue Jan 25, 2018
vsemozhetbyt pushed a commit to nodejs/node that referenced this issue Jan 25, 2018
PR-URL: #18369
Fixes: nodejs/code-and-learn#58
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
evanlucas pushed a commit to nodejs/node that referenced this issue Jan 30, 2018
PR-URL: #18369
Fixes: nodejs/code-and-learn#58
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Feb 27, 2018
PR-URL: #18369
Fixes: nodejs/code-and-learn#58
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18369
Fixes: nodejs/code-and-learn#58
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet