Closed
Bug 753705
Opened 12 years ago
Closed 12 years ago
Increase the timeout in waiting for discovery pane to load
Categories
(Mozilla QA Graveyard :: Mozmill Tests, defect)
Mozilla QA Graveyard
Mozmill Tests
Tracking
(firefox12 fixed, firefox13 fixed, firefox14 fixed, firefox15 fixed, firefox-esr10 fixed, status1.9.2 unaffected)
RESOLVED
FIXED
People
(Reporter: vladmaniac, Assigned: vladmaniac)
Details
(Whiteboard: [lib][qa-])
Attachments
(2 files, 5 obsolete files)
899 bytes,
application/javascript
|
Details | |
3.13 KB,
patch
|
davehunt
:
review+
|
Details | Diff | Splinter Review |
We need to increase the timeout in waiting for disc pane loading This will apply for all the tests which depend on that, or specifically test the discovery pane in any way. Right now, we are talking about remote tests, located under /mozmill-tests/remote/ folder in our repo and addons manager tests located under /mozmill-tests/functional/restartTests Of course we are going to change the timeout value in the API, so the change will apply to the addons.js module. At this initial moment I do not estimate to have any changes in the tests themselves.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → vlad.mozbugs
Status: NEW → ASSIGNED
Assignee | ||
Updated•12 years ago
|
Whiteboard: [lib]
Assignee | ||
Comment 1•12 years ago
|
||
More information about why we need this is located here https://bugzilla.mozilla.org/show_bug.cgi?id=751832#c20
Assignee | ||
Comment 2•12 years ago
|
||
Solution to have a particular case handled for opening disc pane on AOM startup with timeout increased to 30s using the already defined TIMEOUT_REMOTE constant
Attachment #622685 -
Flags: review?(hskupin)
Assignee | ||
Comment 3•12 years ago
|
||
Attaching test
Comment 4•12 years ago
|
||
Comment on attachment 622685 [details] [diff] [review] initial patch (v1.0) > if (waitFor) { >- this.controller.waitFor(function () { >- return self.loaded; >- }, "Selected category has been loaded.", TIMEOUT, undefined, this); >+ // Wait longer if loading the discovery pane on startup >+ if (discoveryPaneURL === AMO_DISCOVER_URL_DEFAULT_VALUE) { >+ this.controller.waitFor(function () { >+ return self.loaded; >+ }, "Discovery pane has been loaded.", TIMEOUT_REMOTE, undefined, this); >+ } else { >+ this.controller.waitFor(function () { >+ return self.loaded; >+ }, "Category has been loaded.", TIMEOUT, undefined, this); >+ } First you will probably be able to get the selected category even if the pane hasn't been loaded yet and use that one to check if 'Get Addons' is selected. Further there is no need to have an if/else. Just define a new timeout variable and assign the corresponding value to it.
Attachment #622685 -
Flags: review?(hskupin) → review-
Assignee | ||
Comment 5•12 years ago
|
||
As I see it we need to * check if AOM loads at startup with 'GetAddons' pane * if yes, check if it loads with the disc pane * if yes, increase timeout * if no, let timeout to 5s * if no, let timeout to 5s This would be the thorough approach IMHO
Comment 6•12 years ago
|
||
It's not only for startup. It's all the time the 'Get Add-ons' tab gets opened.
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #6) > It's not only for startup. It's all the time the 'Get Add-ons' tab gets > opened. Right, my bad there, bad english
Assignee | ||
Comment 8•12 years ago
|
||
So do we agree with the steps in comment5 with your additions in comment6?
Comment 9•12 years ago
|
||
Well you should better come up with an update because I don't think both comments make it clear enough for you. If it wouldn't have been ok for me, I had said something.
Assignee | ||
Comment 10•12 years ago
|
||
It seems that we cannot get the category before its loaded. Added a wip patch to demonstrate that, also the simplified test file is still available for testing the helper function refactor
Attachment #622685 -
Attachment is obsolete: true
Attachment #623631 -
Flags: feedback?(hskupin)
Comment 11•12 years ago
|
||
Comment on attachment 623631 [details] [diff] [review] wip patch v1.0 >+ var timeout = spec.timeout || TIMEOUT; There is no spec.timeout and we do not want to have this given that it is handled within this module gracefully. So please don't introduce a new property. > if (waitFor) { > this.controller.waitFor(function () { > return self.loaded; >- }, "Selected category has been loaded.", TIMEOUT, undefined, this); >+ }, "Selected category has been loaded.", timeout, undefined, this); > > tab = this.waitForOpened(); > } What you want is to modify this code something like that: tab = this.waitForOpened(); var categoryID = this.getCategoryId({category: this.selectedCategory}) var timeout = (categoryID === "discover") ? TIMEOUT_REMOTE : TIMEOUT; this.controller.waitFor(function () { return self.loaded; }, "Selected category has been loaded.", timeout); The reason why it wasn't working is that we were waiting for the add-ons manager tab to be open *after* the check if the category has been loaded. The same code also would have to be added/updated in the waitForCategory() method.
Attachment #623631 -
Flags: feedback?(hskupin) → feedback-
Assignee | ||
Comment 12•12 years ago
|
||
Gee, thanks Henrik!
>
> The same code also would have to be added/updated in the waitForCategory()
> method.
From what I see the waitForCategory() method does this already
Pasted a code snippet from the method, bellow
// For the panes 'discover' and 'search' we have to increase the timeout
// because the ViewChanged event is sent after the remote data has been loaded.
if (categoryId === 'discover' || categoryId === 'search')
timeout = TIMEOUT_REMOTE;
Assignee | ||
Comment 13•12 years ago
|
||
* Modified the "if" clause from waitForCategory() method * Added proposed changes * Tested patch with tests from /mozmill-tests/functional/restartTests folder
Attachment #623631 -
Attachment is obsolete: true
Attachment #624354 -
Flags: review?(hskupin)
Comment 14•12 years ago
|
||
Comment on attachment 624354 [details] [diff] [review] patch v2.0 >- if (waitFor) { >- this.controller.waitFor(function () { >- return self.loaded; >- }, "Selected category has been loaded.", TIMEOUT, undefined, this); >+ tab = this.waitForOpened(); Why do you remove this if construct? It's necessary because we do not want to execute that code all the time, but only if we want to wait for the tab and pane loaded. > waitForCategory : function AddonsManager_waitForCategory(aSpec, aActionCallback) { > var spec = aSpec || { }; > var category = spec.category; > var categoryId = this.getCategoryId({category: category}); Mind combining the last two lines into a single one? >- var timeout = spec.timeout || TIMEOUT; You will also have to update the JSDoc comment. >+ var timeout = (categoryId === "discover" || categoryId === "search") ? TIMEOUT_REMOTE : TIMEOUT; Please wrap that line by moving the colon down under the question mark.
Attachment #624354 -
Flags: review?(hskupin) → review-
Assignee | ||
Comment 15•12 years ago
|
||
Fixed. One thing: If you really want those lines merged we should do it in another bug. That was not intended for this patch and will affect all tests if we do that, so I would not recommend it here.
Attachment #624354 -
Attachment is obsolete: true
Attachment #624370 -
Flags: review?(hskupin)
Comment 16•12 years ago
|
||
Comment on attachment 624370 [details] [diff] [review] patch v3.0 >+ }, "Selected category has been loaded.", timeout); >+ } > >- tab = this.waitForOpened(); >- } nit: please remove the empty line introduced here. >- * timeout - Duration to wait for the target state >- * [optional - default: 5s] >+ * > * @param {Function} aActionCallback Same here. > var category = spec.category; > var categoryId = this.getCategoryId({category: category}); >- var timeout = spec.timeout || TIMEOUT; > > if (!category) > throw new Error(arguments.callee.name + ": Category not specified."); Wait. My fault. You should move the line with the categoryId declaration down below this check. So we can leave them separated and at the same time fix another bug. Otherwise it's fine.
Attachment #624370 -
Flags: review?(hskupin) → review-
Assignee | ||
Comment 17•12 years ago
|
||
nits fixed
Attachment #624370 -
Attachment is obsolete: true
Attachment #624378 -
Flags: review?(hskupin)
Comment 18•12 years ago
|
||
Comment on attachment 624378 [details] [diff] [review] patch v4.0 >+ var categoryId = this.getCategoryId({category: category}); >+ > // For the panes 'discover' and 'search' we have to increase the timeout > // because the ViewChanged event is sent after the remote data has been loaded. >- if (categoryId === 'discover' || categoryId === 'search') >- timeout = TIMEOUT_REMOTE; >+ var timeout = (categoryId === "discover" || categoryId === "search") ? TIMEOUT_REMOTE >+ : TIMEOUT; Well, below the comment please so that it's near the timeout declaration. Otherwise it's fine and we could land it.
Attachment #624378 -
Flags: review?(hskupin) → review-
Updated•12 years ago
|
status-firefox12:
--- → affected
status-firefox13:
--- → affected
status-firefox14:
--- → affected
status-firefox15:
--- → affected
Updated•12 years ago
|
status1.9.2:
--- → unaffected
status-firefox-esr10:
--- → affected
Assignee | ||
Comment 19•12 years ago
|
||
Henrik, can you please clarify comment 18? I'm missing the point in "please so that it's near the timeout declaration". Then I can upload the fix immediately and you can land. Thanks :)
status1.9.2:
unaffected → ---
status-firefox-esr10:
affected → ---
status-firefox12:
affected → ---
status-firefox13:
affected → ---
status-firefox14:
affected → ---
status-firefox15:
affected → ---
Assignee | ||
Updated•12 years ago
|
status1.9.2:
--- → unaffected
status-firefox-esr10:
--- → affected
status-firefox12:
--- → affected
status-firefox13:
--- → affected
status-firefox14:
--- → affected
status-firefox15:
--- → affected
Comment 20•12 years ago
|
||
Please declare categoryId right above timeout so it is below the comment. Always think about to combine blocks and that those are not intersected by comments especially long ones as this one.
Assignee | ||
Comment 21•12 years ago
|
||
fixed
Attachment #624378 -
Attachment is obsolete: true
Attachment #625018 -
Flags: review?(hskupin)
Assignee | ||
Comment 22•12 years ago
|
||
Comment on attachment 625018 [details] [diff] [review] patch v5.0 Asking for r from Dave. Dave - please check previous review (from Henrik) to see if the changes are the ones we want
Attachment #625018 -
Flags: review?(hskupin) → review?(dave.hunt)
Comment 23•12 years ago
|
||
Comment on attachment 625018 [details] [diff] [review] patch v5.0 Looks good. I'll land this on default.
Attachment #625018 -
Flags: review?(dave.hunt) → review+
Comment 24•12 years ago
|
||
Landed as: http://hg.mozilla.org/qa/mozmill-tests/rev/1c5f598a157d Let's see this passing before transplanting to other branches and then working on re-enabling the remote tests.
Comment 25•12 years ago
|
||
Landed as: http://hg.mozilla.org/qa/mozmill-tests/rev/ed0971ff5f30 (mozilla-aurora) http://hg.mozilla.org/qa/mozmill-tests/rev/90d228db3fcd (mozilla-beta) http://hg.mozilla.org/qa/mozmill-tests/rev/8dff52a3cc21 (mozilla-release) http://hg.mozilla.org/qa/mozmill-tests/rev/dfc787a5c212 (mozilla-esr10)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Updated•5 years ago
|
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•