Add ability to update TXTRecord

[KhaosT]

Add the ability to update mDNS Advertisement TXTRecord via DNSServiceUpdateRecord.

Currently only tested on OS X 10.10.2

[KhaosT]

Also tested on Raspberry Pi + Raspbian Jessie

[agnat]

Awesome! Thanks. I believe this is the first new feature in ... a very long time gesture. ;)

I'm going to review the code and will comment inline. Could you look into writing a small test for this?

[agnat]

Could you move this to its own file for consistency?

(I know it's odd and I wouldn't do it again... but that's how it is...)

[agnat]

I'm done with my review. Let me know what you think.

[KhaosT]

@agnat sounds great. For test case, where do you want I put that? In the same file as utils/lib/mdns_test.js or a separate file at tests?

[agnat]

Well, the test goes into the tests subdirectory. Either add it to an existing test (`test_functional.js e.g.) or start a new file. I don't mind...

utils/lib/mdns_test.js contains utilities to write mdns tests. test_functional.js is using it...

[KhaosT]

@agnat added the test case. Not sure if there is a way to detect txtRecord update programmatically.

[agnat]

Actually, I'd prefer a real functional test:

1. Start a browser
2. Start an ad and wait for discovery
3. Update the txt record and launch a second browser
4. Verify the new browser actually gets the new record.

... assuming existing browsers don't get notified when the txt record changes.

[KhaosT]

@agnat Yeah, it appears that browser won't emit serviceChanged event when txt record changes. I updated the test.

[agnat]

Awesome, thanks. There still is the issue with the fixed NULL argument I mentioned in my first review. It probably got lost when you moved the code to a new file...

[KhaosT]

Oops that's awkward... I missed that when copying the arguments 😅 Should be good now?

[agnat]

Should be good now?

Yes, indeed. Please squash your commits and we are ready to roll.

Oh, and if you like add yourself to the contributors sections in the README.md and package.json.

[KhaosT]

Done.

[agnat]

Unfortunately the test fails:

test_functional
✔ simple browsing                                       [116/116]
✔ create ads                                            [8/8]
✘ update ad record                                      [7/8]
AssertionError: 'txtRecord' doesn't match
{ [AssertionError: 'txtRecord' doesn't match]
  name: 'AssertionError',
  actual: '1',
  expected: '2',
  operator: '===',
  message: '\'txtRecord\' doesn\'t match',
  generatedMessage: false }
[SKIPPED] write better test for browseThemAll()
✔ browseThemAll()                                       [0/0]
/Users/david/projects/node.js/node_mdns/lib/advertisement.js:74
  dns_sd.DNSServiceUpdateRecord(this.serviceRef, null, 0, txtRecord, 0);
         ^

TypeError: argument 1 must be a DNSServiceRef (sdRef)
    at TypeError (native)
    at Advertisement.updateTXTRecord (/Users/david/projects/node.js/node_mdns/lib/advertisement.js:74:10)
    at null._onTimeout (/Users/david/projects/node.js/node_mdns/tests/test_functional.js:287:10)
    at Timer.listOnTimeout (timers.js:89:15)

It probably is something simple. Could you look into it and do a quick follow-up PR?

[KhaosT]

It appears that calling DNSServiceUpdateRecord will also invoke the callback registered by DNSServiceRegister which resulted setTimeout being invoked twice. It doesn't look like a documented behavior but it happens...

I can fix the test case by adding a variable to check if the callback has been invoked or I can modify advertisement.js to make sure on_service_registered callback only get invoked once. I prefer to fix it in advertisement.js so the current callback will still behaves the same. What's your thought?

And for 'txtRecord' doesn't match, it might be a timing issue. The test passed in my local network. I can try to increase the delay to run the second scan if necessary.

✔ TXTRecordRef                  [31/31]
✔ buildException()              [47/47]
✔ exportConstants()             [3/3]

test_functional
✔ simple browsing                                       [62/62]
✔ create ads                                            [16/16]
✔ update ad record                                      [8/8]
[SKIPPED] write better test for browseThemAll()

[agnat]

Hm, I see... kind of.

It doesn't look like a documented behavior but it happens...

Right. But it does seem reasonable...

I prefer to fix it in advertisement.js so the current callback will still behaves the same. What's your thought?

I'm not sure yet. I think I prefer not to interfere with the callbacks. Since it only modifies the behavior if you use the new API function it's ok I guess. So, just trap it in the test for now.

Regarding the potential timing issue: Let's fix the callback chaos first and then take it from there.

I'm going to revert the merge. That way you can keep working on the same branch.

[agnat]

I'm going to revert the merge. That way you can keep working on the same branch.

Forget that. I keep forgetting how the revert stuff works... Just create a new PR. ;)