671433 - Add notOK() function to Assertions module

Status: VERIFIED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

To check if an Add-on is disabled we currently do:
> assert.ok(!("ACR" in controller.window), "The add-on is disabled");

It would be great if we could have notOK() so we can do:
> assert.notOk("ACR" in controller.window, "The add-on is disabled");

Same goes for expect.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1

As discussed yesterday on IRC with Harth we want to add the notOk method to the Expect class. This patch is for our tests first. A port for Mozmill will happen once it has been checked-in.

Heather, please give feedback if that's ok. All tests are passing at least.

Comment 2

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 545912 [details] [diff] [review]
Patch v1

I'm r+'ing because your code is fine, but this isn't a good change and I wish we wouldn't make it.

"ok" isn't a synonym for "true"; it means "as expected".

> assert.ok(!("ACR" in controller.window), "The add-on is disabled");

means "It's expected that ACR is not in the window". This is exactly what you mean.

> assert.notOk("ACR" in controller.window, "The add-on is disabled");

means, by extension, "It's unexpected that ACR is in the window". While this is true, it's indirect. 

Also, 'ok' is only really a good name because it's classically used in perl's test::more and others. notOk isn't, and strikes me as pretty weird.

Personally, I think both examples kind of suck for clarity, and would have suggested:

var addonDisabled = !("ACR" in controller.window);
assert.ok(addonDisabled, "The add-on is disabled);

That's how ok is really meant to be used, IMO. You determine a state, then verify that the state is expected. Combining those two into one statement is what's making this awkward-looking.

On a side note,

   let condition = !!aValue;

...in ok() isn't a good construction, and I'd really like to see us not do that anymore pretty much ever. 

I don't want to send "foo" into the function and then have the thing claim that I sent "true" or "false," as it will right now. I want to know that I sent in "foo" so I can debug accurately.

Let the language determine truthiness. Don't sanitize things to true/false, and don't compare directly to true/false. It may not cause a code error, but it'll make things a royal pain to debug later.

Comment 3

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Hrm. I just rechecked the code, and it'll report accurately as aValue is used in the ok() message. So my bad on that rant. 

I still don't like that we're sanitizing native truth. It makes analysis and refactoring more complicated. If, for example, we moved reporting into the base comparison function it would've caused a wrong message.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #2)
> I'm r+'ing because your code is fine, but this isn't a good change and I
> wish we wouldn't make it.

I'm struggling too as more as I think about it. There are no real complex use cases which would require such a method, and there are other ways of doing it more coherently.

> "ok" isn't a synonym for "true"; it means "as expected".
> 
> > assert.ok(!("ACR" in controller.window), "The add-on is disabled");
> 
> means "It's expected that ACR is not in the window". This is exactly what
> you mean.
> 
> > assert.notOk("ACR" in controller.window, "The add-on is disabled");
> 
> means, by extension, "It's unexpected that ACR is in the window". While this
> is true, it's indirect. 

Good feedback. I would second on it after reading.

> Personally, I think both examples kind of suck for clarity, and would have
> suggested:
> 
> var addonDisabled = !("ACR" in controller.window);
> assert.ok(addonDisabled, "The add-on is disabled);

In this case I would even say we could abuse equal and use it that way:

expect.equal("ACR" in controller.window, false, "The add-on is disabled");

So I would vote to WONTFIX this bug. Heather and Clint, can you both give some additional feedback?

Comment 5

[Heather Arthur [:harth]]

Geo raises a really good point. Going to WONTFIX.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks all for the feedback. Lets go without this method.