TODO/XXX/FIXME comments in tests

[Trott]

Ref: #264

There are 11 TODO, XXX, and FIXME comments in the test directory. It would be great to either remove them from the code (if they are no longer valid or at least not particularly high value), or get issues opened for them, or just get whatever it is they are addressing addressed. Here they are as of this writing. (Actually, I'm kind of cheating and leaving out two that @santigimeno has PRs in to fix already.)

• 1: test/internet/test-dgram-multicast-multi-process.js:

  var sendSocket = dgram.createSocket('udp4');
  // FIXME: a libuv limitation makes it necessary to bind()
  // before calling any of the set*() functions. The bind()
  // call is what creates the actual socket.

EDIT: This one is being discussed in #5023

• 2: test/parallel/test-child-process-fork-net2.js:

      disconnected += 1;
    });
    // XXX This resume() should be unnecessary.
    // a stream high water mark should be enough to keep
    // consuming the input.
  

• 3: test/parallel/test-cluster-disconnect-handles.js:

  let isKilling = false;
  const handles = require('internal/cluster').handles;
  // FIXME(bnoordhuis) lib/cluster.js scans the execArgv arguments for
  // debugger flags and renumbers any port numbers it sees starting
  // from the default port 5858. Add a '.' that circumvents the

• 4: test/parallel/test-cluster-http-pipe.js:

  http.createServer(function(req, res) {
  assert.equal(req.connection.remoteAddress, undefined);
  assert.equal(req.connection.localAddress, undefined); // TODO common.PIPE?
  res.writeHead(200);
  res.end('OK');

• 5: test/parallel/test-dgram-oob-buffer.js:

  });

  socket.close(); // FIXME should not be necessary

• 6: test/parallel/test-domain-multi.js:

  });

  // XXX this bind should not be necessary.
  // the write cb behavior in http/net should use an
  // event so that it picks up the domain handling.

• 7: test/parallel/test-punycode.js:

  // (I) Russian (Cyrillic)
  /* XXX disabled, fails - possibly a bug in the RFC
  'b1abfaaepdrnnbgefbaDotcwatmq2g4l':
  '\u043F\u043E\u0447\u0435\u043C\u0443\u0436\u0435\u043E\u043D\u0438' +

• 8: test/parallel/test-repl-persistent-history.js:

           '4', '2', '\'', '\'42\'\n', prompt, prompt],
  

  after: function ensureHistoryFixture() {
  // XXX(Fishrock123) Make sure nothing weird happened to our fixture
  // or it's temporary copy.
  // Sometimes this test used to erase the fixture and I'm not sure why.

• 9: test/pummel/test-tls-securepair-client.js:

  function test(keyfn, certfn, check, next) {
  // FIXME: Avoid the common PORT as this test currently hits a C-level
  // assertion error with node_g. The program aborts without HUPing
  // the openssl s_server thus causing many tests to fail with

• 10: test/parallel/test-setproctitle.js:

  // Original test written by Jakub Lekstan kuebzky@gmail.com

  // FIXME add sunos support
  if ('linux freebsd darwin'.indexOf(process.platform) === -1) {
  console.log(1..0 # Skipped: Unsupported platform [${process.platform}]);

• 11: test/sequential/test-zerolengthbufferbug.js:

  var server = http.createServer(function(req, res) {
  var buffer = new Buffer(0);
  // FIXME: WTF gjslint want this?
  res.writeHead(200, {'Content-Type': 'text/html',
  'Content-Length': buffer.length});

/cc @bnoordhuis @Fishrock123

[Fishrock123]

8: test/parallel/test-repl-persistent-history.js:
// XXX(Fishrock123) Make sure nothing weird happened to our fixture

In older versions of the test (pre-git) I pointed directly to the fixture, but it ended up erasing it after the run. I am really not sure why. (Something we now do to the temporary file but did not do at the time the comment was written.)

[aayn]

I'd like to do this please(as it's tagged 'good first contribution'). So, do I just remove the comments?

[Fishrock123]

@aayn Not quite -- you'd need to do some research in order to check if the comment is still relevant, then decide what to do. Myself or @Trott can probably guide you if you need any help along the way. :)

[aayn]

Okay. I'll get to it right away. :)
But yeah, I'll probably need some guidance.

[aayn]

For 7: test/parallel/test-punycode.js, the Russian(Cyrillic) test still fails, so an issue should be opened for it probably.

[aayn]

For 2: test/parallel/test-child-process-fork-net2.js, seems like the client.resume() really is unnecessary. To be sure, I ran it 10,000 times without the client.resume() using a script and it never failed.

update: So, both the comment and the resume() can be removed.

[aayn]

For 4: test/parallel/test-cluster-http-pipe.js, I think the // TODO common.PIPE can be removed straight away. It most likely refers to some common.PIPE implementation which already seems to be done.
Even if it doesn't, that comment is not very helpful and should be removed anyway.

[Fishrock123]

I'd like @bnoordhuis to comment on 4., once he is well again. The origin commit shows some signifigance: de88255

Edit: It was re-added there. The true origin was 296b7a5#diff-92a4de49a056f9e6d4d14bd396d326b0R45, but I think the commits that removed it and then were reverted are more telling?

[aayn]

In 5: test/parallel/test-dgram-oob-buffer.js, I'm not entirely sure why closing a socket is unnecessary. Seems fine to me. If it is indeed fine, then just removing the comment would do.

[aayn]

11: test/sequential/test-zerolengthbufferbug.js The comment should just be removed IMO. Not a very helpful comment, but it's probably referring to some linting issues. This is what eslint test-zerolengthbufferbug.js throws:

1:1 error Definition for rule 'new-with-error' was not found new-with-error
1:1 error Definition for rule 'required-modules' was not found required-modules

update: @Trott Yup. That command works. My bad. Turns out that comment(thanks for introducing me to git blame) was written by 'Oleg Efimov' all the way back in 2010. But, I still can't understand what the comment is referring to.

[Trott]

@aayn Those lint issues are because (for whatever reason) your eslint is not loading the custom rules that are specified in the project .eslintrc and contained in tools/eslint-rules. They are unrelated to the contents of the file.

EDIT: This command should work from the top-level directory of the project:

node tools/eslint/bin/eslint.js src lib test tools/eslint-rules \
        --rulesdir tools/eslint-rules --quiet

[Trott]

@aayn It might be good (if you haven't) to use git blame to find out when that line was introduced and see if there's any clue in the relevant pull request or elsewhere that might explain what is going on with that comment.

EDIT: Referring to 11 in this case.

[aayn]

For 10: test/sequential/test-setproctitle.js, the test file considers anything other than linux, freebsd, or darwin as not supported. The commenter, I believe @bnoordhuis, mentioned that sunos support be added too. If that's been added, we could add sunos to the string with other platforms and then remove the comment. Else, the comment still stands.

edit: @Trott Is there support for sunos? If not, probably an issue should be opened. Else, I could remove the comment and add sunos to the list.

[aayn]

Running 3: test/parallel/test-cluster-disconnect-handles.js fails for me. It says:
Error: Cannot find module 'internal/cluster' and a bunch of other stuff. Is that a problem just on my side?

update: @Trott, thanks. Works fine with the flag.

[Trott]

@aayn If you look at the top of that file, you'll see this comment:

// Flags: --expose_internals

That let's the test harness know that the test needs to be run with the --expose_internals flag.

./node --expose_internals test/parallel/test-cluster-disconnect-handles.js 

[aayn]

@Trott I don't quite understand the rest of the tests. Could you point me to some resources/code or tell me a bit about them?

[Trott]

@aayn If the tests themselves are not self-explanatory through their code and comments, I don't think they tend to be documented elsewhere. I usually will use git history and/or git blame to try to find the commit and/or PR where they were introduced and see if I can figure out what's going on from that.

Standardizing a way to document tests is (or should be) on a long list of activities for the Testing WG but I don't think we/they will get to it anytime too soon.

[aayn]

@Trott For 1(which seems to be written by you), I get the following as the output:

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

Do I need to make a socket listen or something?(Sorry if I'm asking a noob question 😅)

[Trott]

@aayn Although I am the last person to touch that comment, it actually originates with @bnoordhuis in 2012 with 46e86aa8. A lot has changed since 2012 and it's possible that the comment is obsolete. Hopefully he can shed some light on it (although like everyone everywhere, he's no doubt got a lot of other things vying for his attention, so we may have to be patient).

Regarding getting the test to work: My naive analysis would be that it's possible multicast UDP is disabled on your computer or that you are having firewall issues.

[aayn]

@Trott Oh yeah. It was something to do with my iptables configuration. Now, it works.

[aayn]

So, in 1, seems like the bind() is not necessary any more. The set*() functions work just fine without the bind().

[aayn]

So, here is the summary of changes suggested by me so far:

• In 1, remove the sendSocket.bind() along with the comments below.
• In 2, remove the resume() and the comment below.
• In 5, just remove the comments, as closing a socket seems fine to me.
• Open an issue for 7 as the mentioned test still fails.

@Trott If you concur, can I make these changes please( I'd really like to make a contribution 😅 )? I'll suggest changes on other files as I get to know more about node and when @bnoordhuis replies.

[Trott]

@aayn Pick the one that you feel the most confident about, and open a pull request for it. Then you can take what you learn from the process and apply it to the others.

Or open a pull request for each of the first three and an issue for the fourth one if you want.

Each of these changes should probably be its own pull request (or, for the last one, issue). Other than being TODO comments, they're not related, so it's probably best not to bundle them.

[ALJCepeda]

It seems that subtask 3 was addressed in c5c28c3

[jasnell]

@Trott ... any reason to keep this open?

[Trott]

(copy/pasted from a related issue)

@jasnell I'm OK with closing this and the other TODO/XXX/FIXME issues. If any of those items are things that really ought to be fixed (rather than a wishlist or a "will fix after Magical Feature X is available"), a separate issue should be opened anyway because it's just getting lost in these out-of-date tracking issues.

While I think this issue is superfluous personally, anyone else should feel free to re-open (if GitHub permits them to) or comment requesting this be re-opened.