Last Comment Bug 787024 - Replace controller.waitFor with assert/expect.waitFor in tests/*/restartTests
: Replace controller.waitFor with assert/expect.waitFor in tests/*/restartTests
Status: RESOLVED FIXED
s=q3 u=failure c=mozmill p=1
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Maniac Vlad Florin (:vladmaniac)
:
:
Mentors:
Depends on: 787020
Blocks: 786306
  Show dependency treegraph
 
Reported: 2012-08-30 05:55 PDT by Alex Lakatos[:AlexLakatos]
Modified: 2012-09-25 00:34 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed
fixed
fixed


Attachments
[initial patch] patch v1.0 (8.02 KB, patch)
2012-09-18 01:30 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
patch v1.1 (9.51 KB, patch)
2012-09-18 05:17 PDT, Maniac Vlad Florin (:vladmaniac)
no flags Details | Diff | Splinter Review
patch v1.2 (17.00 KB, patch)
2012-09-20 00:06 PDT, Maniac Vlad Florin (:vladmaniac)
dave.hunt: review-
Details | Diff | Splinter Review
patch v1.3 (16.99 KB, patch)
2012-09-20 23:30 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review+
Details | Diff | Splinter Review
[mozilla-aurora] patch v1.0 (16.43 KB, patch)
2012-09-24 05:00 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
[mozilla-beta][mozilla-release] patch v1.0 (16.98 KB, patch)
2012-09-24 05:19 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review+
Details | Diff | Splinter Review
[mozilla-esr10] patch v1.0 (16.68 KB, patch)
2012-09-24 05:40 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review+
Details | Diff | Splinter Review
[mozilla-aurora]patch v1.1 (16.97 KB, patch)
2012-09-24 23:33 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review+
Details | Diff | Splinter Review

Description Alex Lakatos[:AlexLakatos] 2012-08-30 05:55:56 PDT
Replace soft assertions with hard assertions in tests/functional/restartTests. Needed for bug 786306 and bug 787020.
Comment 1 Alex Lakatos[:AlexLakatos] 2012-08-30 06:02:33 PDT
The scope changes, we need to replace controller.waitFor with assert/expect.waitFor in tests/functional/restartTests
Comment 2 Henrik Skupin (:whimboo) 2012-09-17 11:46:01 PDT
What is the status of this bug? It's in waiting mode for the last week since bug 787020 has been fixed.
Comment 3 Henrik Skupin (:whimboo) 2012-09-18 00:37:56 PDT
It's a dependency for bug 786306 so it should have been fixed alongside. But for now lets put this into the task list.
Comment 4 Maniac Vlad Florin (:vladmaniac) 2012-09-18 00:41:51 PDT
I will solve this today
Comment 5 Maniac Vlad Florin (:vladmaniac) 2012-09-18 01:09:08 PDT
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.
Comment 6 Henrik Skupin (:whimboo) 2012-09-18 01:23:40 PDT
This will apply to all testrun types.
Comment 7 Maniac Vlad Florin (:vladmaniac) 2012-09-18 01:30:31 PDT
Created attachment 662067 [details] [diff] [review]
[initial patch] patch v1.0

* 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 8 Henrik Skupin (:whimboo) 2012-09-18 03:08:27 PDT
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.
Comment 9 Maniac Vlad Florin (:vladmaniac) 2012-09-18 03:13:59 PDT
> >  
> >    // 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.
Comment 10 Henrik Skupin (:whimboo) 2012-09-18 03:34:54 PDT
So can you ensure that if the bookmarks toolbar is not visible the remaining tests in this method will not fail?
Comment 11 Maniac Vlad Florin (:vladmaniac) 2012-09-18 03:46:51 PDT
(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
Comment 12 Henrik Skupin (:whimboo) 2012-09-18 05:02:14 PDT
(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.
Comment 13 Maniac Vlad Florin (:vladmaniac) 2012-09-18 05:10:19 PDT
(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
Comment 14 Maniac Vlad Florin (:vladmaniac) 2012-09-18 05:17:12 PDT
Created attachment 662129 [details] [diff] [review]
patch v1.1

* 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 15 Henrik Skupin (:whimboo) 2012-09-18 05:31:16 PDT
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.
Comment 16 Maniac Vlad Florin (:vladmaniac) 2012-09-18 05:38:55 PDT
(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
Comment 17 Henrik Skupin (:whimboo) 2012-09-18 06:03:24 PDT
(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.
Comment 18 Maniac Vlad Florin (:vladmaniac) 2012-09-20 00:06:16 PDT
Created attachment 662832 [details] [diff] [review]
patch v1.2

* 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 19 Dave Hunt (:davehunt) 2012-09-20 06:50:08 PDT
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.
Comment 20 Henrik Skupin (:whimboo) 2012-09-20 09:35:16 PDT
(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.
Comment 21 Maniac Vlad Florin (:vladmaniac) 2012-09-20 23:27:50 PDT
Filed bug 793092
Comment 22 Maniac Vlad Florin (:vladmaniac) 2012-09-20 23:30:30 PDT
Created attachment 663306 [details] [diff] [review]
patch v1.3

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 23 Henrik Skupin (:whimboo) 2012-09-21 01:57:21 PDT
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.
Comment 24 Henrik Skupin (:whimboo) 2012-09-21 01:58:51 PDT
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.
Comment 25 Maniac Vlad Florin (:vladmaniac) 2012-09-24 05:00:48 PDT
Created attachment 664019 [details] [diff] [review]
[mozilla-aurora] patch v1.0

* backport patch for mozilla-aurora branch. this is necessary because of the remote test folder being different here.
Comment 26 Maniac Vlad Florin (:vladmaniac) 2012-09-24 05:19:11 PDT
Created attachment 664024 [details] [diff] [review]
[mozilla-beta][mozilla-release] patch v1.0

* backport for beta and release
Comment 27 Maniac Vlad Florin (:vladmaniac) 2012-09-24 05:40:20 PDT
Created attachment 664029 [details] [diff] [review]
[mozilla-esr10] patch v1.0

* backport patch for mozilla-esr10
Comment 28 Henrik Skupin (:whimboo) 2012-09-24 13:11:14 PDT
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.
Comment 29 Henrik Skupin (:whimboo) 2012-09-24 14:11:20 PDT
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.
Comment 30 Maniac Vlad Florin (:vladmaniac) 2012-09-24 23:33:25 PDT
Created attachment 664373 [details] [diff] [review]
[mozilla-aurora]patch v1.1

omg imported the assertion module and tested the whole refactoring patch.we are ok now
Comment 31 Henrik Skupin (:whimboo) 2012-09-25 00:34:08 PDT
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)

Note You need to log in before you can comment on or make changes to this bug.