1313745 - Use Assert.jsm in IRC code

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[Patrick Cloke [:clokep]]

The main change here is using Assert.jsm assertions in the IRC test code.

This also fixes a lot of linting errors that existed.

Comment 1

[Patrick Cloke [:clokep]]

Attachment: assert-irc.diff

Comment 2

[aleth [:aleth]]

Comment on attachment 8805629 [details] [diff] [review]
assert-irc.diff

Review of attachment 8805629 [details] [diff] [review]:
-----------------------------------------------------------------

You're working towards enabling the linter? Nice!

::: chat/protocols/irc/ircCTCP.jsm
@@ +146,5 @@
>          // Received a CLIENTINFO request, respond with the support CTCP
>          // messages.
>          let info = new Set();
>          for (let handler of ircHandlers._ctcpHandlers) {
> +          for (let command of Object.keys(handler.commands))

Why this change?

::: chat/protocols/irc/ircDCC.jsm
@@ +35,5 @@
>      message.ctcp.dcc = {
>        type: params[0],
>        argument: params[1],
> +      address: Number(params[2]),
> +      port: Number(params[3]),

I don't really like this change. You do want to invoke the constructor, and iirc doing it without new is nonstandard. Why are you modifying it?

(similarly in other files)

::: chat/protocols/irc/test/test_ircMessage.js
@@ -151,5 @@
>  }
>  
> -/*
> - * Test if two objects have the same fields (recursively). aObject2 should be
> - * an ircMessage and aObject1 its expected values.

ah ha! another one of these helpers ;)

::: chat/protocols/irc/test/test_ircNonStandard.js
@@ +109,3 @@
>  
>      // Don't try to authenticate with NickServ.
> +    equal(account.shouldAuthenticate, false);

strictly speaking: strictEqual

Comment 3

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #2)
> > +      port: Number(params[3]),
> 
> I don't really like this change. You do want to invoke the constructor, and
> iirc doing it without new is nonstandard. Why are you modifying it?

22:23:15 - aleth: Are linters that expect "Number(x)" instead of "new Number(x)" in the wrong?
22:25:15 - arai: aleth: those have different meaning
22:25:25 - arai: https://tc39.github.io/ecma262/#sec-number-constructor-number-value
22:26:01 - arai: aleth: so, suggesting "new" regardless of the context is wrong
22:26:40 - arai: aleth: "Number(x)" returns number, "new Number(x)" returns object
22:26:55 - aleth: arai, evilpie: Thanks! I was under the impression generically using object(x) was just a nonstandard way of invoking the constructor
22:27:54 - evilpie: aleth: arai: in general yes, new things added to the spec, that aren't primitives will require `new`, but Number/String/Boolean etc. are special

Comment 4

[aleth [:aleth]]

Comment on attachment 8805629 [details] [diff] [review]
assert-irc.diff

Review of attachment 8805629 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with an answer to the first question in comment 2 (I assume you just wanted own properties but I'm not sure if there was a specific reason for it)

Comment 5

[Patrick Cloke [:clokep]]

(In reply to aleth [:aleth] from comment #4)
> Comment on attachment 8805629 [details] [diff] [review]
> assert-irc.diff
> 
> Review of attachment 8805629 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with an answer to the first question in comment 2 (I assume you just
> wanted own properties but I'm not sure if there was a specific reason for it)

Yes, the linter was complaining to me here, but maybe that's not a good reason. I can back out those parts of the changes if you want?

Comment 6

[aleth [:aleth]]

(In reply to Patrick Cloke [:clokep] from comment #5)
> Yes, the linter was complaining to me here, but maybe that's not a good
> reason. I can back out those parts of the changes if you want?

I don't particularly mind in this case, but it seems strange that the linter would choke on for...in. Fallen might know if there's a specific rule about that?

Comment 7

[Patrick Cloke [:clokep]]

Attachment: Patch v2

This gets rid of the Object.keys usage.

Comment 8

[Patrick Cloke [:clokep]]

Comment on attachment 8830279 [details] [diff] [review]
Patch v2

Review of attachment 8830279 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like I have one thing to check still, sorry for the noise!

::: chat/protocols/irc/ircCTCP.jsm
@@ +146,5 @@
>          // Received a CLIENTINFO request, respond with the support CTCP
>          // messages.
>          let info = new Set();
>          for (let handler of ircHandlers._ctcpHandlers) {
> +          for (let command of handler.commands)

I'm not sure this change is correct.

Comment 9

[Patrick Cloke [:clokep]]

Attachment: Patch v2.1

Removes the line in ircCTCP.jsm.

Comment 10

[Patrick Cloke [:clokep]]

Thanks!

https://hg.mozilla.org/comm-central/rev/8e836ed136c6a62d90ba5e8ba754564f66e1b588