Closed Bug 868371 Opened 11 years ago Closed 11 years ago

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

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

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

RESOLVED FIXED
Tracking Status
firefox22 --- fixed
firefox23 --- fixed
firefox24 --- fixed
firefox25 --- fixed
firefox-esr17 --- fixed

People

(Reporter: daniela.p98911, Assigned: daniela.p98911)

References

Details

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

Attachments

(3 files, 4 obsolete files)

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.
Whiteboard: [lib]
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]
Attached patch patch v1.0 (obsolete) — Splinter Review
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+
(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+
Attached patch patch v1.1 (obsolete) — Splinter Review
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)
Blocks: 744007
No longer blocks: 868375
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-
Attached patch patch v1.2 (obsolete) — Splinter Review
(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-
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-
Attached patch patch v1.4Splinter Review
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.
Whiteboard: [lib][mentor=andreeamatei][lang=js][sprint2013-30] → [lib][sprint2013-30]
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)
(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.
(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.
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
Closed: 11 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: