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.

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

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?

(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.

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

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.

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.

(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

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.

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

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.

Modified patch based on review.

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.

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.

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

(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!