Closed Bug 787024 Opened 9 years ago Closed 9 years ago

Replace controller.waitFor with assert/expect.waitFor in tests/*/restartTests

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(firefox15 fixed, firefox16 fixed, firefox17 fixed, firefox18 fixed, firefox-esr10 fixed)

RESOLVED FIXED
Tracking Status
firefox15 --- fixed
firefox16 --- fixed
firefox17 --- fixed
firefox18 --- fixed
firefox-esr10 --- fixed

People

(Reporter: AlexLakatos, Assigned: vladmaniac)

References

Details

(Whiteboard: s=q3 u=failure c=mozmill p=1)

Attachments

(4 files, 4 obsolete files)

Replace soft assertions with hard assertions in tests/functional/restartTests. Needed for bug 786306 and bug 787020.
Summary: Replace soft assertions with hard assertions in tests/functional/restartTests → Replace controller.waitFor with assert/expect.waitFor in tests/functional/restartTests
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.
Whiteboard: s=q3 u=failure c=mozmill p=1
I will solve this today
Assignee: nobody → vlad.mozbugs
Status: NEW → ASSIGNED
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.
Summary: Replace controller.waitFor with assert/expect.waitFor in tests/functional/restartTests → Replace controller.waitFor with assert/expect.waitFor in tests/*/restartTests
Attached patch [initial patch] patch v1.0 (obsolete) — Splinter Review
* 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
Attachment #662067 - Flags: review?(hskupin)
Attachment #662067 - Flags: review?(dave.hunt)
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.
Attachment #662067 - Flags: review?(hskupin)
Attachment #662067 - Flags: review?(dave.hunt)
Attachment #662067 - Flags: review-
> >  
> >    // 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
Attached patch patch v1.1 (obsolete) — Splinter Review
* 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
Attachment #662067 - Attachment is obsolete: true
Attachment #662129 - Flags: review?(hskupin)
Attachment #662129 - Flags: review?(dave.hunt)
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.
Attachment #662129 - Flags: review?(hskupin)
Attachment #662129 - Flags: review?(dave.hunt)
(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.
Attached patch patch v1.2 (obsolete) — Splinter Review
* fixed to change the software-update.js file and update restart tests
* passed internal reviews
* was tested across platforms both by me and reviewers
Attachment #662129 - Attachment is obsolete: true
Attachment #662832 - Flags: review?(hskupin)
Attachment #662832 - Flags: review?(dave.hunt)
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.
Attachment #662832 - Flags: review?(hskupin)
Attachment #662832 - Flags: review?(dave.hunt)
Attachment #662832 - Flags: review-
(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.
Attached patch patch v1.3Splinter Review
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
Attachment #662832 - Attachment is obsolete: true
Attachment #663306 - Flags: review?(hskupin)
Attachment #663306 - Flags: review?(dave.hunt)
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.
Attachment #663306 - Flags: review?(hskupin)
Attachment #663306 - Flags: review?(dave.hunt)
Attachment #663306 - Flags: review+
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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attached patch [mozilla-aurora] patch v1.0 (obsolete) — Splinter Review
* backport patch for mozilla-aurora branch. this is necessary because of the remote test folder being different here.
Attachment #664019 - Flags: review?(hskupin)
Attachment #664019 - Flags: review?(dave.hunt)
* backport for beta and release
Attachment #664024 - Flags: review?(hskupin)
Attachment #664024 - Flags: review?(dave.hunt)
* backport patch for mozilla-esr10
Attachment #664029 - Flags: review?(hskupin)
Attachment #664029 - Flags: review?(dave.hunt)
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.
Attachment #664019 - Flags: review?(hskupin)
Attachment #664019 - Flags: review?(dave.hunt)
Attachment #664019 - Flags: review-
Attachment #664024 - Flags: review?(hskupin)
Attachment #664024 - Flags: review?(dave.hunt)
Attachment #664024 - Flags: review+
Attachment #664029 - Flags: review?(hskupin)
Attachment #664029 - Flags: review?(dave.hunt)
Attachment #664029 - Flags: review+
omg imported the assertion module and tested the whole refactoring patch.we are ok now
Attachment #664019 - Attachment is obsolete: true
Attachment #664373 - Flags: review?(hskupin)
Attachment #664373 - Flags: review?(dave.hunt)
Attachment #664373 - Flags: review?(hskupin)
Attachment #664373 - Flags: review?(dave.hunt)
Attachment #664373 - Flags: review+
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)
Status: REOPENED → RESOLVED
Closed: 9 years ago9 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.