868375 - Change the assertions.js module so that Assert is the base class instead of Expect

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Daniela Petrovici]

This is based on bug 791634 comment #6. 

The Assert class in assertions.js module has to be the base one so that we can move the code from utils.waitFor to the assertions module. Also, if Assert is the base class, in Expect we will catch the assertions exceptions.

Comment 1

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

As said, we should do this first for our assertion module in mozmill-tests to verify it works as expected.

Comment 2

[Daniela Petrovici]

Attachment: patch v1.0

Reports:
With changes to assert:
Linux:
http://mozmill-crowd.blargon7.com/#/functional/report/f2d3b6136b11cb7b6708636cff6a6e5d
MAC:
http://mozmill-crowd.blargon7.com/#/functional/report/f2d3b6136b11cb7b6708636cff695c9b

Clean no changes at all:
Linux:
http://mozmill-crowd.blargon7.com/#/functional/report/f2d3b6136b11cb7b6708636cff695580

MAC:
http://mozmill-crowd.blargon7.com/#/functional/report/f2d3b6136b11cb7b6708636cff694a1f

Comment 3

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

Comment on attachment 763524 [details] [diff] [review]
patch v1.0

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

Looks nearly ok, just a missed line.

::: mozmill/mozmill/extension/resource/modules/assertions.js
@@ -537,5 @@
> -var Assert = function () {}
> -
> -Assert.prototype = new Expect();
> -
> -Assert.prototype.AssertionError = AssertionError;

Where did this line went to? We still need the object attached to the Assert class.

Comment 4

[Daniela Petrovici]

Attachment: patch v1.1

Modified patch based on review.
Reports are:
Linux:
http://mozmill-crowd.blargon7.com/#/functional/report/f2d3b6136b11cb7b6708636cff852289
MAC:
http://mozmill-crowd.blargon7.com/#/functional/report/f2d3b6136b11cb7b6708636cff86831d

Comment 5

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

Comment on attachment 764050 [details] [diff] [review]
patch v1.1

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

::: mozmill/mozmill/extension/resource/modules/assertions.js
@@ +571,1 @@
>  Assert.prototype.AssertionError = AssertionError;

Please put this directly into the prototype definition of the Assert class. If AssertionError needs to be moved up before any other class, I would even support that for clarity.

Comment 6

[Daniela Petrovici]

Attachment: patch v1.2

Modified patch based on review.

Reports are below. I will also attach the log file with console output for checking the assert.AssertionError works the same now.

Linux:
http://mozmill-crowd.blargon7.com/#/functional/report/f2d3b6136b11cb7b6708636cffdc3eb2
MAC:
http://mozmill-crowd.blargon7.com/#/functional/report/f2d3b6136b11cb7b6708636cffdcde03

Comment 7

[Daniela Petrovici]

Attachment: log with output for a mutt test failing

This is a log file where I have dumped assert.AssertionError (before and after the change) and also modified controller/contextmenu.js mutt test to fail to compare if it works the same in both cases

Comment 8

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

(In reply to Daniela Petrovici from comment #7)
> This is a log file where I have dumped assert.AssertionError (before and
> after the change) and also modified controller/contextmenu.js mutt test to
> fail to compare if it works the same in both cases

This is not what you want to test. What you really want is to catch such an exception and check that it's of type assert.AssertionError.

Comment 9

[Daniela Petrovici]

Comment on attachment 764720 [details] [diff] [review]
patch v1.2

Testing now based on the previous comment

Comment 10

[Daniela Petrovici]

Attachment: patch v1.3

(In reply to Henrik Skupin (:whimboo) from comment #8)
> This is not what you want to test. What you really want is to catch such an
> exception and check that it's of type assert.AssertionError.
I used try-catch and caught an error from assert.equal and dumped ex instanceof assert.AssertionError. It shows true.
I also tried dumping the constructor.name for the exception and this change did not affect anything.

Also, the mutt tests are still working properly, including the mutt test for assertions

Comment 11

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

Comment on attachment 764737 [details] [diff] [review]
patch v1.3

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

One last thing to do, then we should be good in getting this landed.

::: mozmill/mozmill/extension/resource/modules/assertions.js
@@ +251,5 @@
>  
>    /**
> +   * Error object thrown by the Assert class
> +   */
> +  AssertionError: AssertionError,

This is an attribute on the prototype, so it should be located before any getters, setter, and methods.

Comment 12

[Daniela Petrovici]

Attachment: patch v1.4

Modified patch based on review

Comment 13

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

Landed on master:
https://github.com/mozilla/mozmill/commit/df77fa4b82c54340777fe55bd5bac90b06f85f86