Make changes to assertions.js library to make Assert the base class

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Daniela Petrovici, Assigned: Daniela Petrovici)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(firefox22 fixed, firefox23 fixed, firefox24 fixed, firefox25 fixed, firefox-esr17 fixed)

Details

(Whiteboard: [lib][sprint2013-30])

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
This is logged based on bug 791634 comment #6. We need to make Assert the base class so that Expect should catch possible assertions. 

This change needs to be done in the hg.mozilla.org/qa/mozmill-tests/file/default/lib/assertions.js library.
(Assignee)

Updated

5 years ago
Whiteboard: [lib]
Blocks: 868375

Updated

5 years ago
Whiteboard: [lib] → [lib][sprint2013-30]
Assignee: nobody → andreea.matei
Status: NEW → ASSIGNED
Assignee: andreea.matei → nobody
Status: ASSIGNED → NEW
Whiteboard: [lib][sprint2013-30] → [lib][mentor=andreeamatei][lang=js][sprint2013-30]
(Assignee)

Comment 1

5 years ago
Created attachment 757917 [details] [diff] [review]
patch v1.0

This is the patch that changes the base class in assertions.js library to be Assert instead of Expect. 

Reports for:
- Linux: 
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a2f242
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a2d77d
- Windows:
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a2e607
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a2f0fe
- MAC: 
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a301da
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a30809

To test that nothing changed in the current functionality I have used:
- a clean repo where I have modified some tests to fail for methods in Assert and Expect classes.
- a repo where I have changed the base class and also modified the same tests to fail for methods in Assert and Expect classes.

Failure reports for:
1) repo without changes to assertions.js are below:
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a26d10
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a29406
MAC: http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a2ac7d
2) repo with changes to assertions.js to make Assert the base class are below:
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a27612
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a2b87c
MAC: http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a2ce7f
Assignee: nobody → dpetrovici
Status: NEW → ASSIGNED
Attachment #757917 - Flags: feedback?(hskupin)
Attachment #757917 - Flags: feedback?(dave.hunt)
Comment on attachment 757917 [details] [diff] [review]
patch v1.0

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

::: lib/assertions.js
@@ +58,5 @@
> +  let condition = true;
> +  let message = aMessage;
> +
> +  try {
> +    mozmill.utils.waitFor(aCallback, aMessage, aTimeout, aInterval, aThisObject);

Is there a separate bug for switching this to use assert.waitFor?
Attachment #757917 - Flags: feedback?(hskupin)
Attachment #757917 - Flags: feedback?(dave.hunt)
Attachment #757917 - Flags: feedback+
(Assignee)

Comment 3

5 years ago
(In reply to Dave Hunt (:davehunt) from comment #2)
> > +    mozmill.utils.waitFor(aCallback, aMessage, aTimeout, aInterval, aThisObject);
> 
> Is there a separate bug for switching this to use assert.waitFor?
No, I will change it to use assert.waitFor here.
(In reply to Daniela Petrovici from comment #3)
> (In reply to Dave Hunt (:davehunt) from comment #2)
> > > +    mozmill.utils.waitFor(aCallback, aMessage, aTimeout, aInterval, aThisObject);
> > 
> > Is there a separate bug for switching this to use assert.waitFor?
> No, I will change it to use assert.waitFor here.

Why that? That work is part of bug 791634 and as we agreed on it is blocked by getting a patch on this bug landed first.
Comment on attachment 757917 [details] [diff] [review]
patch v1.0

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

Overall it looks fine. I gave already some comments for the general structure. More to come once a review request is up.

::: lib/assertions.js
@@ +23,5 @@
>   *
>   * @class Base class for non-fatal assertions
>   */
>  function Expect() {
>  }

Please move all Expect declarations below the Assert class.

@@ +73,5 @@
> + * when a failing test has to directly abort the current test function. All
> + * remaining tasks will not be performed.
> + *
> + * @class Base class for fatal assertions
> + * @extends assertions.Expect

This needs an update and has to be replaced with the doc around the Expect constructor.
Attachment #757917 - Flags: feedback+
(Assignee)

Comment 6

5 years ago
Created attachment 758467 [details] [diff] [review]
patch v1.1

Modified patch based on feedback.

Report: http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916b2ce61

(In reply to Henrik Skupin (:whimboo) from comment #4)
> Why that? That work is part of bug 791634 and as we agreed on it is blocked
> by getting a patch on this bug landed first.
Actually this issue is for mozmill-tests where assert.waitFor is already implemented. Bug 791634 is for mozmill 2.0 where were need to implement waitFor.

Based on bug 868375 comment #1 we need to first make this change in mozmill-tests for mozmill 1.5, then we will implement it in mozmill 2.0
Attachment #757917 - Attachment is obsolete: true
Attachment #758467 - Flags: review?(dave.hunt)
Attachment #758467 - Flags: review?(andreea.matei)
(Assignee)

Updated

5 years ago
Blocks: 744007
No longer blocks: 868375
(Assignee)

Updated

5 years ago
Blocks: 868375
Comment on attachment 758467 [details] [diff] [review]
patch v1.1

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

Looks good to me, but I'll hold off landing this based on Henrik's comment:

> More to come once a review request is up.
Attachment #758467 - Flags: review?(hskupin)
Attachment #758467 - Flags: review?(dave.hunt)
Attachment #758467 - Flags: review?(andreea.matei)
Attachment #758467 - Flags: review+
Comment on attachment 758467 [details] [diff] [review]
patch v1.1

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

r- mostly because I see two failures when running the /lib/tests/test_assertions.js test. That one will have to pass too, given that's in our unit test suite for library modules.

::: lib/assertions.js
@@ -136,5 @@
> - * @param {String} aResult.message Message why the assertion failed.
> - */
> -Expect.prototype._logFail = function Expect__logFail(aResult) {
> -  mozmillFrame.events.fail({fail: aResult});
> -};

Please move Assert.prototype._logFail up to this line, so that we have that code side by side.
Attachment #758467 - Flags: review?(hskupin) → review-
(Assignee)

Comment 9

5 years ago
Created attachment 759729 [details] [diff] [review]
patch v1.2

(In reply to Henrik Skupin (:whimboo) from comment #8)
> Comment on attachment 758467 [details] [diff] [review]
> patch v1.1
> 
> Review of attachment 758467 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- mostly because I see two failures when running the
> /lib/tests/test_assertions.js test. That one will have to pass too, given
> that's in our unit test suite for library modules.
This is because of the SoftExpect that extends the Expect class. this.parent.waitFor call is not working as expected: it should work as:
- testExpect calling Expect.waitFor that will call Assert.waitFor
but it really works like this:
Expect.waitFor is being called twice (once from testExpect in test_assertions.js and once from Expect.waitFor), then Assert.waitFor is called.

Since using this.parent.waitFor inside the Expect.waitFor method is not the correct approach and it makes the test_assertions fail, I have discussed with Dave and decided to leave this change out. The code in Expect.waitFor will not be changed in this bug as this is just for making the Assert the base class.

Regarding the changing of the waitFor method, I will start a discussion thread about how to best implement this.

Reports are below:
Linux:
Passing runs:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe53c52f
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe51a5b9
Failing - no changes to assertions.js:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe4fa015
Failing - changes to assertions.js:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe50aef7

MAC:
Passing runs:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe526596
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe540e0d
Failing - no changes to assertions.js:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe512056
Failing - changes to assertions.js:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe5585d7


Windows:
Passing runs:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe534a61
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe54ef17
Failing - no changes to assertions.js:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe4fd3a5
Failing - changes to assertions.js:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe5593b5
Attachment #758467 - Attachment is obsolete: true
Attachment #759729 - Flags: review?(hskupin)
Comment on attachment 759729 [details] [diff] [review]
patch v1.2

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

::: lib/assertions.js
@@ +529,5 @@
> +}
> +
> +Expect.prototype = new Assert();
> +Expect.prototype.constructor = Expect;
> +Expect.prototype.parent = Assert.prototype;

I think we should get rid of parent. It's not necessary and we will most likely never have to make use of it. Instead the Expect.waitFor() method has to call Assert.waitFor.call(this, ...). Only that will care about the correct class you want to call the method on.

So please make that change here and update Expect.waitFor() appropriately. Also fix the test for the assertions module similarly.
Attachment #759729 - Flags: review?(hskupin) → review-
(Assignee)

Comment 11

5 years ago
Created attachment 760952 [details] [diff] [review]
patch v1.3

Made changes to Expect.waitFor.

Reports for passing runs:
Windows:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbeaa036f
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbeaa3c25
MAC:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbeba6a4c
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbebbdb4d
Linux:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbeb27047
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbeb2e431

Reports for runs where I have made some tests fail in repo's with and without the patch are below:
Linux:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbeae4405
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbeaec649
MAC:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbebc0483
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbebd6d05
Windows:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbebf6134
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbebf7a00
Attachment #759729 - Attachment is obsolete: true
Attachment #760952 - Flags: review?(hskupin)
Comment on attachment 760952 [details] [diff] [review]
patch v1.3

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

Ok, this looks fine. Only some minor adjustments to do.

::: lib/assertions.js
@@ +125,5 @@
>    return true;
>  };
>  
>  /**
> + * Log a test as failing by throwing an AssertionException.

We do not throw an AssertionException.

@@ +133,5 @@
>   * @param {String} aResult.fileName Name of the file in which the assertion failed.
>   * @param {String} aResult.function Function in which the assertion failed.
>   * @param {Number} aResult.lineNumber Line number of the file in which the assertion failed.
>   * @param {String} aResult.message Message why the assertion failed.
> + * @throws {Error}

We have to add this to nearly all the methods now for the Assertion class which call _logFail directly or indirectly.

@@ +560,5 @@
> +  let message = aMessage;
> +
> +  try {
> +    Assert.prototype.waitFor.call(this, aCallback, aMessage, aTimeout, aInterval, aThisObject);
> +  } catch (ex) {

No hanging catch statement please.
Attachment #760952 - Flags: review?(hskupin) → review-
(Assignee)

Comment 13

5 years ago
Created attachment 762641 [details] [diff] [review]
patch v1.4

Modified patch based on review.
Attachment #760952 - Attachment is obsolete: true
Attachment #762641 - Flags: review?(hskupin)
Comment on attachment 762641 [details] [diff] [review]
patch v1.4

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

Looks fine now. I will get this landed. Can you please start on the patch for Mozmill itself? Thanks.
Attachment #762641 - Flags: review?(hskupin) → review+
Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/6f85bc8d4ede

Given the importance of that change I think we should get this backported. Lets see how it sticks.
status-firefox21: --- → affected
status-firefox22: --- → affected
status-firefox23: --- → affected
status-firefox24: --- → fixed
status-firefox-esr17: --- → affected
Whiteboard: [lib][mentor=andreeamatei][lang=js][sprint2013-30] → [lib][sprint2013-30]
(Assignee)

Comment 16

5 years ago
Created attachment 764042 [details] [diff] [review]
patch v1.0 for Aurora, Beta, Release and ESR17

The patch for default does not apply cleanly on Aurora. The patch for Aurora applies cleanly on Beta, ESR17 and Release branches. I will attach the reports in a moment
Attachment #764042 - Flags: review?(andreea.matei)
(Assignee)

Comment 17

5 years ago
Created attachment 764043 [details]
reports for passing runs and failing runs with and without the changes to assertions
(In reply to Daniela Petrovici from comment #16)
> The patch for default does not apply cleanly on Aurora. The patch for Aurora
> applies cleanly on Beta, ESR17 and Release branches. I will attach the
> reports in a moment

Why doesn't is apply cleanly on those branches? I don't think that the file is different, given that we want to keep it in sync across branches.
(Assignee)

Comment 19

5 years ago
(In reply to Henrik Skupin (:whimboo) from comment #18)
> Why doesn't is apply cleanly on those branches? I don't think that the file
> is different, given that we want to keep it in sync across branches.
The comments were changed for default branch in bug 872918. The changes made in that bug were not backported on other branches.

These are the lines that are different between default and other branches:
http://hg.mozilla.org/qa/mozmill-tests/file/default/lib/assertions.js#l348
http://hg.mozilla.org/qa/mozmill-tests/file/mozilla-aurora/lib/assertions.js#l331
And
http://hg.mozilla.org/qa/mozmill-tests/file/default/lib/assertions.js#l379
http://hg.mozilla.org/qa/mozmill-tests/file/mozilla-aurora/lib/assertions.js#l360
Andreea, can you please get this reviewed and backported? Thanks.
status-firefox21: affected → ---
Comment on attachment 764042 [details] [diff] [review]
patch v1.0 for Aurora, Beta, Release and ESR17

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

Landed on:
http://hg.mozilla.org/qa/mozmill-tests/rev/f9ac097bcf36 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/31322f79a81f (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/1d5a033b1533 (esr17)

Aurora was already merged. Thanks!
Attachment #764042 - Flags: review?(andreea.matei) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox22: affected → fixed
status-firefox23: affected → fixed
status-firefox25: --- → fixed
status-firefox-esr17: affected → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.