[mozmill] Update tests to use nsIBrowserHistory.removeAllPages()

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: whimboo, Assigned: geo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozmill-test-failure][mozmill-doc-needed])

Attachments

(2 attachments, 1 obsolete attachment)

All of our location bar and bookmark tests are using PlacesAPI.historyService.removeAllPages() to clear the history. Surprisingly that shouldn't even work because the function doesn't exist. We would have to use nsIBrowserHistory.removeAllPages instead.

A good example can be found here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_tabMatchesInAwesomebar.js#218

We simply would need that function duplicated into our PlacesAPI and tests should call it instead of the interface function directly.

Geo, can you please update the API and the tests? That is probably one of the failures we see but don't throw an exception in setupModule, which should always be done.
Created attachment 452158 [details] [diff] [review]
Change references to PlacesAPI.historyService.removeAllPages() to use nsIBrowserHistory and async handling

* Added browserHistory variable to PlacesAPI module.  I intentionally instantiated it the way I did (as opposed to off of the existing historyService var) to stay close to the canon in the docs.

* Added PlacesAPI.removeAllHistory(), which wraps browserHistory.removeAllPages().  removeAllPages() is async, so this blocks on the observer being called back.

* Changed all references to PlacesAPI.historyService.removeAllPages() to use PlacesAPI.removeAllHistory() and removed any unconditional exception handling around the original calls.

I imagine removeAllPages() does exist in an undocumented state on historyService.  I say that mostly because I tested the call w/o the exception handlers, and it resolved.  It might be there to fulfill the nsIBrowserHistory interface.  Either way, it's definitely better to go with the documented method.
Attachment #452158 - Flags: review?(hskupin)
One more detail I almost forgot.  I wrote this for trunk, but also checked it against 3.6.3.  It hung on 3.6.3 in the waitFor (guessing the API wasn't async then) so check before backporting.
(In reply to comment #2)
> One more detail I almost forgot.  I wrote this for trunk, but also checked it
> against 3.6.3.  It hung on 3.6.3 in the waitFor (guessing the API wasn't async
> then) so check before backporting.

Marco, is that the case? Have we moved removeAllPages from sync to async for Firefox 4.0? Otherwise we will have to check our helper function why we hang.
Comment on attachment 452158 [details] [diff] [review]
Change references to PlacesAPI.historyService.removeAllPages() to use nsIBrowserHistory and async handling

Overall it looks great! Just have a single item here we will have to figure out before I want to checkin the patch.

>+  // Remove the pages, then block until we're done or until timeout is reached
>+  browserHistory.removeAllPages();
>+  controller.waitForEval("subject.state == true", undefined, undefined, 
>+                         finishedFlag);
>+}

Please don't use undefined for both values. For the timeout part we should define a global constant for that shared module. Because we don't have much history for our tests a timeout of 5 seconds should be more than enough. As interval we are using 100ms.

r=me with the changes discussed and applied.
Attachment #452158 - Flags: review?(hskupin) → review+
(In reply to comment #3)
> (In reply to comment #2)
> > One more detail I almost forgot.  I wrote this for trunk, but also checked it
> > against 3.6.3.  It hung on 3.6.3 in the waitFor (guessing the API wasn't async
> > then) so check before backporting.
> 
> Marco, is that the case? Have we moved removeAllPages from sync to async for
> Firefox 4.0? Otherwise we will have to check our helper function why we hang.

yes, Firefox 3.6 does not have async expiration.
Thanks Marco. So lets create different patches for Minefield and older branches then.
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][mozmill-doc-needed]
No prob, re: constants.  I used undefined to preserve the default behavior of the waitFor (30 secs/100ms, btw) while still supplying the subject.  If you want to lock that in, I propose we put those in something other than PlacesAPI--perhaps a global constants module or something.  waitFors seem to come up a lot in functional automation.

Other than that, this patch should be fine for default/trunk.  The info about 3.6.3 explains why existing code doesn't use async---I can make a different version of the function with no async for older branches.

Which mozmill branch should I make the variant patch against?
(In reply to comment #7)
> No prob, re: constants.  I used undefined to preserve the default behavior of
> the waitFor (30 secs/100ms, btw) while still supplying the subject.  If you
> want to lock that in, I propose we put those in something other than
> PlacesAPI--perhaps a global constants module or something.  waitFors seem to
> come up a lot in functional automation.

Right now each module has the gTimeout constant set. Lets follow-up with the same style for now. We will do some refactoring next quarter. For now please create the constant with the above name and use 5000ms. We don't want to wait 30 seconds.

> Other than that, this patch should be fine for default/trunk.  The info about
> 3.6.3 explains why existing code doesn't use async---I can make a different
> version of the function with no async for older branches.
> 
> Which mozmill branch should I make the variant patch against?

Always the next one. Means in this case mozilla1.9.2. It should hopefully cleanly apply to mozilla1.9.1.
OK, cool.  Hadn't realized we'd already established a standard, and agreed re: 5s vs. 30s.  

I'll get those changes in a new default branch patch presently, to unblock abillings, and will follow up with the patch against 1.9.2 somewhat after.
Created attachment 452329 [details] [diff] [review]
Followup to first review [checked-in]

Attached the followup patch.  This is strictly for default.  I'll add another patch within 30 mins-1 hr to handle 1.9.2 and before.
Attachment #452158 - Attachment is obsolete: true
Attachment #452329 - Flags: review?(hskupin)
Comment on attachment 452329 [details] [diff] [review]
Followup to first review [checked-in]

r=me

Patch has been landed on default branch:
http://hg.mozilla.org/qa/mozmill-tests/rev/3e20aa9473c3
Attachment #452329 - Attachment description: Followup to first review from Henrik, uses gTimeout constant and explicit refresh interval. → Followup to first review [checked-in]
Attachment #452329 - Flags: review?(hskupin) → review+
Created attachment 452411 [details] [diff] [review]
Same as above patch, but synchronous removeAllHistory() function for 1.9.2 and 1.9.1

Per previous discussion, this patch is identical to the previous, but with the removeAllHistory() function just being an adapter.

So, some good news/bad news.  Good news is that TextMate has a strip trailing whitespace option.  Bad news is that it's whole file or nothing, and the editor will insert whitespace for the indent on every new blank line.  I massaged this patch, but that's not really going to be a viable solution for either of us to do going forward.

We should figure out what to do about this; TextMate is one of the top programmer's editors on the Mac, so we can't just say "use something else" (plus I don't wanna :)  Plus, I'm sure some other editors have this behavior as well.

Obvious possibilities are 1) don't worry about it, at least within reason, since it's not a particularly disruptive thing; or 2) strip the existing files and maintain that standard going forward (in which case I enable strip-on-save in TextMate).
Attachment #452411 - Flags: review?(hskupin)
Comment on attachment 452411 [details] [diff] [review]
Same as above patch, but synchronous removeAllHistory() function for 1.9.2 and 1.9.1

>+++ b/firefox/testAwesomeBar/testEscapeAutocomplete.js
>-  try {
>-    PlacesAPI.historyService.removeAllPages();
>-  } catch (ex) {}
>-}
>+  PlacesAPI.removeAllHistory();

Well, that's the only problem I saw. I have fixed it directly on 1.9.1 and pushed a bustage fix for 1.9.2, because I have seen it too late.

Otherwise looks good.
Attachment #452411 - Flags: review?(hskupin) → review+
(In reply to comment #4)
> >+  controller.waitForEval("subject.state == true", undefined, undefined, 
> >+                         finishedFlag);

Well, inside of shared modules we will have to prefix this call with "mozmill". Otherwise it will fail. I have pushed a bustage fix to the default branch:

http://hg.mozilla.org/qa/mozmill-tests/rev/9400443310be
Mass move of Mozmill Test related project bugs to newly created components. You can filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.
Component: Bookmarks & History → Mozmill Tests
Product: Firefox → Mozilla QA
QA Contact: bookmarks → mozmill-tests
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.