Closed Bug 1261233 Opened 5 years ago Closed 4 years ago

Intermittent test_bug331215.xul | application timed out after 330 seconds with no output

Categories

(Toolkit :: General, defect, P3)

Product:
Toolkit
Component:
General
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: KWierso, Assigned: mikedeboer)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Bug 1261233 - prevent test_bug331215.xul from intermittently timing out.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Summary: Intermittent test_bug331215.xul | application timed out after 330 seconds with no output | application crashed [@ mach_msg_trap + 0xa] → Intermittent test_bug331215.xul | application timed out after 330 seconds with no output
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
Mike, this and bug 1259339 seem to be hitting again. Can you please take a look?
Flags: needinfo?(mdeboer)
See Also: → 1259339
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: needinfo?(mdeboer)
Comment on attachment 8835079 [details]
Bug 1261233 - prevent test_bug331215.xul from intermittently timing out.

https://reviewboard.mozilla.org/r/110796/#review112190

Looks good to me, thanks for making our tests better. Some suggestions below. :-)

::: toolkit/content/tests/chrome/bug331215_window.xul:51
(Diff revision 1)
>  
>      function* startTestWithBrowser(browserId) {
>        info("Starting test with browser '" + browserId + "'");
>        gBrowser = document.getElementById(browserId);
>        gFindBar.browser = gBrowser;
> -      let promise = ContentTask.spawn(gBrowser, null, function* () {
> +      let promise = BrowserTestUtils.browserLoaded(gBrowser);

I think you want gBrowser.selectedBrowser. Not sure if it actually works if you pass tabbrowser, but might as well be precise. :-)

::: toolkit/content/tests/chrome/bug331215_window.xul:66
(Diff revision 1)
>                .getControllerForCommand("cmd_moveTop")
>                .doCommand("cmd_moveTop");
> -      yield enterStringIntoFindField("l");
> +      yield promiseEnterStringIntoFindField("l");
>        ok(gFindBar._findField.getAttribute("status") == "notfound",
>           "Findfield status attribute should have been 'notfound'" +
>           " after entering ltest");              

Nit: can we fix the trailing ws here while we're here?

::: toolkit/content/tests/chrome/bug331215_window.xul:74
(Diff revision 1)
>      }
>  
> -    function* enterStringIntoFindField(aString) {
> +    function promiseEnterStringIntoFindField(aString) {
> -      for (let i = 0; i < aString.length; i++) {      
> -        let event = document.createEvent("KeyboardEvent");
> -        let promise = new Promise(resolve => {
> +      let promise = new Promise(resolve => {

Nit: just use:

```js
return new Promise(resolve => {
  ...
});
```

::: toolkit/content/tests/chrome/bug331215_window.xul:86
(Diff revision 1)
> -            }
> +          }
> -          };
> +        };
> -          gFindBar.browser.finder.addResultListener(listener);
> +        gFindBar.browser.finder.addResultListener(listener);
> -        });
> +      });
> +
> +      for (let i = 0; i < aString.length; i++) {      

Nit: trailing ws

Also, you can just actually use a:

```js
for (let c of aString) {
```
style loop (yeah! iterate over characters!)

Also, can we use a new KeyEvent/KeyboardEvent() instead of using `createEvent` and would that allow us to just pass the string instead of the charCode? That'd be simpler. The constructor should definitely be simpler than the initKeyEvent method. :-)
Attachment #8835079 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8835079 [details]
Bug 1261233 - prevent test_bug331215.xul from intermittently timing out.

https://reviewboard.mozilla.org/r/110796/#review112190

> I think you want gBrowser.selectedBrowser. Not sure if it actually works if you pass tabbrowser, but might as well be precise. :-)

I understand the confusement; gBrowser is used differently than in browser/ Fx code and mochitest-browser tests - here it's simply used as the holder of the current browser element.

> Nit: trailing ws
> 
> Also, you can just actually use a:
> 
> ```js
> for (let c of aString) {
> ```
> style loop (yeah! iterate over characters!)
> 
> Also, can we use a new KeyEvent/KeyboardEvent() instead of using `createEvent` and would that allow us to just pass the string instead of the charCode? That'd be simpler. The constructor should definitely be simpler than the initKeyEvent method. :-)

Excellent review comments, thanks!
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f1fa60cbe6a
prevent test_bug331215.xul from intermittently timing out. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/8f1fa60cbe6a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.