Replace soft assertions with hard assertions in tests/functional/restartTests. Needed for bug 786306 and bug 787020.

The scope changes, we need to replace controller.waitFor with assert/expect.waitFor in tests/functional/restartTests

What is the status of this bug? It's in waiting mode for the last week since bug 787020 has been fixed.

It's a dependency for bug 786306 so it should have been fixed alongside. But for now lets put this into the task list.

I will solve this today

As asked on #automation today, do we need this also for remote/restartTests? Think we should update those as well while we are at it.

This will apply to all testrun types.

* so this passed internal review from Andreea * no whitespaces * I did not replace any of the messages in the waitFors, please point to that in reviews if necessary * Updated both enabled or diabled tests * currently this patch will apply for default, I have not tested the other branches, will do once we have settled a good patch to backport

Comment on attachment 662067 [details] [diff] [review] [initial patch] patch v1.0 Review of attachment 662067 [details] [diff] [review]: ----------------------------------------------------------------- Looks like you missed the update tests here which are also restart tests. That would also include its library module. Can you please file a follow-up bug so we can cover this change for all the other code too? ::: tests/functional/restartTests/testDefaultBookmarks/test1.js @@ +46,5 @@ > controller.mouseDown(toggle); > controller.mouseUp(toggle); > > // Make sure bookmarks toolbar is now open > + expect.waitFor(function() { Why have you made it an expect.waitFor() call? I would like to hear the reason for. ::: tests/remote/restartTests/testDiscoveryPane_FeaturedModule/test1.js @@ +85,3 @@ > return !installButton.getNode().disabled; > }, "Install button is enabled: got '" + !installButton.getNode().disabled + > "', expected 'true'"); Would you mind to remove the got part of the message? That's not something we can do for a waitFor() message. This also applies to the other restart tests in this testrun.

> > > > // Make sure bookmarks toolbar is now open > > + expect.waitFor(function() { I will answer this one before moving on with the revisions. The test is about checking the default bookmarks, not the bookmarks toolbar state. More, we can check state of default bookmarks in many other ways (like using backend API) without even touching the bookmarks toolbar. So re those reason, I consider the waitFors to be an expect.waitFor.

So can you ensure that if the bookmarks toolbar is not visible the remaining tests in this method will not fail?

(In reply to Henrik Skupin (:whimboo) from comment #10) > So can you ensure that if the bookmarks toolbar is not visible the remaining > tests in this method will not fail? I haven't properly checked but at least the elements can be accessed even if the toolbar is not visible. we do not click anything on the toolbar in this test, if we clicked, than we would have failed because we depended on the toolbar and items visibility, IMO

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #11) > I haven't properly checked but at least the elements can be accessed even if > the toolbar is not visible. we do not click anything on the toolbar in this Invasive changes always require a deep testing. Not only in case of the expected behavior but also for negative situations.

(In reply to Henrik Skupin (:whimboo) from comment #12) > (In reply to Maniac Vlad Florin (:vladmaniac) from comment #11) > > I haven't properly checked but at least the elements can be accessed even if > > the toolbar is not visible. we do not click anything on the toolbar in this > > Invasive changes always require a deep testing. Not only in case of the > expected behavior but also for negative situations. You're right. So after looking closer I can say for sure that the above assert snippet will fail if we have no visibility on the bookmarks toolbar. controller.assert(function() { return items.length == 2; }, "Bookmarks Toolbar contains 2 items"); As you were right in the first place, I'm going to change the expect calls into assert calls, as it should be

* we have assert calls for checking the bookmarks toolbar state in testDefaultBookmarks folder * deleted the 'got' calls as advice, they were only present in remote restart tests

Comment on attachment 662129 [details] [diff] [review] patch v1.1 Review of attachment 662129 [details] [diff] [review]: ----------------------------------------------------------------- I don't think that went through an internal review because the update tests are still missing in this patch.

(In reply to Henrik Skupin (:whimboo) from comment #15) > Comment on attachment 662129 [details] [diff] [review] > patch v1.1 > > Review of attachment 662129 [details] [diff] [review]: > ----------------------------------------------------------------- > > I don't think that went through an internal review because the update tests > are still missing in this patch. "Looks like you missed the update tests here which are also restart tests. That would also include its library module. Can you please file a follow-up bug so we can cover this change for all the other code too?" in comment 8 made us understand we need the updates tests and its library module in another bug. I will make the appropriate changes in a follow up patch then

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #16) > in comment 8 made us understand we need the updates tests and its library > module in another bug. I will make the appropriate changes in a follow up > patch then No, this bug is about restart tests which include update tests and its shared module. Other code means all the other test modules and libs which are not involved in restart tests.

* fixed to change the software-update.js file and update restart tests * passed internal reviews * was tested across platforms both by me and reviewers

Comment on attachment 662832 [details] [diff] [review] patch v1.2 Review of attachment 662832 [details] [diff] [review]: ----------------------------------------------------------------- The commit message needs to be updated to not just reference functional restart tests. Are we planning on doing the same for non restart tests? If so, do we have a bug raised for that? The reason I ask is that the endurance tests are also a form of restart tests, so we should consider including them in this patch. I'm happy for them to be taken care of separately though.

(In reply to Dave Hunt (:davehunt) from comment #19) > Are we planning on doing the same for non restart tests? If so, do we have a > bug raised for that? The reason I ask is that the endurance tests are also a Yes, we do. And I have requested a follow-up bug for that 2 days ago. But it hasn't been raised yet. See comment 8. > form of restart tests, so we should consider including them in this patch. > I'm happy for them to be taken care of separately though. I would take care of them in the other bug so we are unblocked here for the new feature in restart tests.

Filed bug 793092

thanks for review Dave! fixed the commit message to point out exactly what the patch covers otherwise there are no functional modifications in the new patch, since were not requested in the review

Comment on attachment 663306 [details] [diff] [review] patch v1.3 Review of attachment 663306 [details] [diff] [review]: ----------------------------------------------------------------- I think that looks good now. Lets see how it sticks on default.

http://hg.mozilla.org/qa/mozmill-tests/rev/dc0249046e24 (default) If the testruns today pass we can backport. So please check if the patch applies cleanly.

* backport patch for mozilla-aurora branch. this is necessary because of the remote test folder being different here.

* backport for beta and release

* backport patch for mozilla-esr10

Comment on attachment 664019 [details] [diff] [review] [mozilla-aurora] patch v1.0 Review of attachment 664019 [details] [diff] [review]: ----------------------------------------------------------------- ::: tests/remote/restartTests/testDiscoveryPane_FeaturedModule/test1.js @@ +79,5 @@ > var installButton = new elementslib.Lookup(controller.window.document, > '/id("xpinstallConfirm")/anon({"anonid":"buttons"})' + > '/{"dlgtype":"accept"}'); > > + assert.waitFor(function () { The assert method has not been imported for this test so this will fail.

http://hg.mozilla.org/qa/mozmill-tests/rev/fc7972edfbdc (beta) http://hg.mozilla.org/qa/mozmill-tests/rev/82e29ba0b037 (release) http://hg.mozilla.org/qa/mozmill-tests/rev/ac4cde9b093d (esr10) Reopening until the remaining patch for Aurora has been checked-in.

omg imported the assertion module and tested the whole refactoring patch.we are ok now

Again, not sure why you have started to use the branch name in commit messages. But I have updated it myself again. Please stop doing that. Thanks. http://hg.mozilla.org/qa/mozmill-tests/rev/830f3b7c1f20 (aurora)