Closed Bug 1446671 Opened 6 years ago Closed 6 years ago

Intermittent accessible/tests/mochitest/textcaret/test_browserui.xul | Test timed out.

Categories

(Core :: Disability Access APIs, defect, P5)

Product:
Core
Component:
Disability Access APIs
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: surkov)

References

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:other])

Attachments

(1 file, 1 obsolete file)

patch2
5.62 KB, patch
Details | Diff | Splinter Review 
Filed by: archaeopteryx [at] coole-files.de

https://treeherder.mozilla.org/logviewer.html#?job_id=168701247&repo=try

https://queue.taskcluster.net/v1/task/VhwXIMPmS4KWHxs2wn3HeQ/runs/0/artifacts/public/logs/live_backing.log

[task 2018-03-17T12:19:50.577Z] 12:19:50     INFO - TEST-START | accessible/tests/mochitest/textcaret/test_browserui.xul
[task 2018-03-17T12:19:50.796Z] 12:19:50     INFO - GECKO(1050) | WARNING: content window passed to PrivateBrowsingUtils.isWindowPrivate. Use isContentWindowPrivate instead (but only for frame scripts).
[task 2018-03-17T12:19:50.799Z] 12:19:50     INFO - GECKO(1050) | pbu_isWindowPrivate@resource://gre/modules/PrivateBrowsingUtils.jsm:26:14
[task 2018-03-17T12:19:50.800Z] 12:19:50     INFO - GECKO(1050) | windowPrivacyMatches@chrome://browser/content/browser-sidebar.js:235:12
[task 2018-03-17T12:19:50.800Z] 12:19:50     INFO - GECKO(1050) | startDelayedLoad@chrome://browser/content/browser-sidebar.js:248:10
[task 2018-03-17T12:19:50.801Z] 12:19:50     INFO - GECKO(1050) | _delayedStartup/<@chrome://browser/content/browser.js:1596:7
[task 2018-03-17T12:25:06.467Z] 12:25:06     INFO - TEST-INFO | started process screentopng
[task 2018-03-17T12:25:06.773Z] 12:25:06     INFO - TEST-INFO | screentopng: exit 0
[task 2018-03-17T12:25:06.775Z] 12:25:06     INFO - Buffered messages logged at 12:19:50
[task 2018-03-17T12:25:06.775Z] 12:25:06     INFO - must wait for load
[task 2018-03-17T12:25:06.776Z] 12:25:06     INFO - must wait for focus
[task 2018-03-17T12:25:06.776Z] 12:25:06     INFO - Buffered messages finished
[task 2018-03-17T12:25:06.777Z] 12:25:06     INFO - TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/textcaret/test_browserui.xul | Test timed out. 
[task 2018-03-17T12:25:06.777Z] 12:25:06     INFO - reportError@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:121:7
[task 2018-03-17T12:25:06.778Z] 12:25:06     INFO - TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:142:7
[task 2018-03-17T12:25:07.465Z] 12:25:07     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-03-17T12:25:07.468Z] 12:25:07     INFO - TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/textcaret/test_browserui.xul | [SimpleTest.finish()] waitForFocus() was called a different number of times from the number of callbacks run.  Maybe the test terminated prematurely -- be sure to use SimpleTest.waitForExplicitFinish(). - got 1, expected +0
(In reply to Sebastian Hengst - PTO, back April 3rd [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #1)
> This doesn't show up on the latest push on inbound (a merge) with 40
> retriggers but the previous push:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-
> searchStr=842b88bf096c0ff5a87996328af7d9dc6d865353&fromchange=97160a734959af7
> 3cc97af0bf8d198e301ebedae&tochange=efce78e62b6dac195e7cb4898684da54155d1661&g
> roup_state=expanded&selectedJob=168736020
> Issue outside the tree, e.g. newly set up machines?

Recent inbound failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=169616927&repo=mozilla-inbound&lineNumber=7391
There are 52 failures associated to this bug in the last 7 days. This affects mochitests on Linux 32&64 mostly opt and some pgo build types.

surkov: can you take a look at this?
Flags: needinfo?(surkov.alexander)
Whiteboard: [stockwell needswork]
(In reply to alexander :surkov from comment #8)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> e4c4f0ee3e74bf899d1fee024abfd067b6ec2509
> Bug 1446671 - enable logging for textcaret/test_browserui.xul

let's see if it gives us any additional info, at least the relevant section doesn't give any clue and also it's not different from normal runs.

[task 2018-04-01T13:01:06.138Z] 13:01:06     INFO - TEST-START | accessible/tests/mochitest/textcaret/test_browserui.xul
[task 2018-04-01T13:01:06.421Z] 13:01:06     INFO - GECKO(1042) | WARNING: content window passed to PrivateBrowsingUtils.isWindowPrivate. Use isContentWindowPrivate instead (but only for frame scripts).
[task 2018-04-01T13:01:06.421Z] 13:01:06     INFO - GECKO(1042) | pbu_isWindowPrivate@resource://gre/modules/PrivateBrowsingUtils.jsm:26:14
[task 2018-04-01T13:01:06.434Z] 13:01:06     INFO - GECKO(1042) | windowPrivacyMatches@chrome://browser/content/browser-sidebar.js:235:12
[task 2018-04-01T13:01:06.434Z] 13:01:06     INFO - GECKO(1042) | startDelayedLoad@chrome://browser/content/browser-sidebar.js:248:10
[task 2018-04-01T13:01:06.434Z] 13:01:06     INFO - GECKO(1042) | _delayedStartup/<@chrome://browser/content/browser.js:1563:7
[task 2018-04-01T13:01:06.434Z] 13:01:06     INFO - GECKO(1042) | promise callback*_delayedStartup@chrome://browser/content/browser.js:1554:5
[task 2018-04-01T13:01:06.435Z] 13:01:06     INFO - GECKO(1042) | EventListener.handleEvent*onLoad@chrome://browser/content/browser.js:1376:5
[task 2018-04-01T13:01:06.436Z] 13:01:06     INFO - GECKO(1042) | onload@chrome://browser/content/browser.xul:1:1
[task 2018-04-01T13:06:15.577Z] 13:06:15     INFO - TEST-INFO | started process screentopng
[task 2018-04-01T13:06:15.888Z] 13:06:15     INFO - TEST-INFO | screentopng: exit 0
[task 2018-04-01T13:06:15.888Z] 13:06:15     INFO - Buffered messages logged at 13:01:06
[task 2018-04-01T13:06:15.890Z] 13:06:15     INFO - must wait for load
[task 2018-04-01T13:06:15.890Z] 13:06:15     INFO - Buffered messages finished
[task 2018-04-01T13:06:15.891Z] 13:06:15     INFO - TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/textcaret/test_browserui.xul | Test timed out.
Flags: needinfo?(surkov.alexander)
Keywords: leave-open
See Also: → 1429575
can you please look at the additional logging and fix/disable this test?
Flags: needinfo?(surkov.alexander)
Attached patch patch (obsolete) — — Splinter Review 
Assignee: nobody → surkov.alexander
Flags: needinfo?(surkov.alexander)

        Attachment #8966344 -
        Flags: review?(yzenevich)

        Attachment #8966344 -
        Flags: review?(gijskruitbosch+bugs)

    
    

    
  
Comment on attachment 8966344 [details] [diff] [review]
patch

Review of attachment 8966344 [details] [diff] [review]:
-----------------------------------------------------------------

FWIW, there seems to be no patch header here with a commit message, and also there's almost no context on the diffs. It would be nice to fix this in your setup, because reviewing e.g. the BrowserTestUtils change is quite painful like this - there's no context to see what you're doing.

Tentative r=me but please do fix the issues below, especially the skip-if, after which you may want a new tryrun if you've done one already.

::: accessible/tests/browser/events/browser.ini
@@ +10,1 @@
>  skip-if = e10s

You're not running this test almost anywhere now, because it's skip-if=e10s, whereas the docload test is no longer skipped. I assume you want to insert it 1 line down.

::: accessible/tests/browser/events/browser_test_textcaret.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Nit: the original test was not licensed, and we normally license tests PD. Example header:


/* Any copyright is dedicated to the Public Domain.
 * http://creativecommons.org/publicdomain/zero/1.0/ */

@@ +19,5 @@
> +    return cmEvent.accessible == getAccessible(target) && cmEvent.caretOffset == caretOffset;
> +  }
> +}
> +
> +async function runTests(browser, accDoc) {

Both of these arguments seem unused. Could you just use a normal add_task() ? Is there anything specific you need from the helper?

Unrelated, nit: please name the task something more descriptive than 'runTests', maybe something like 'checkURLBarCaretEvents' or whatever

        Attachment #8966344 -
        Flags: review?(gijskruitbosch+bugs) → review+

    
    

    
  
Comment on attachment 8966344 [details] [diff] [review]
patch

Review of attachment 8966344 [details] [diff] [review]:
-----------------------------------------------------------------

Some eslint errors:

...browser_test_textcaret.js
  20:4   error  Missing semicolon.             semi (eslint)
  37:12  error  Strings must use doublequote.  quotes (eslint)
  40:65  error  Unexpected trailing comma.     comma-dangle (eslint)
  45:12  error  Strings must use doublequote.  quotes (eslint)

    
    

    
  
Comment on attachment 8966344 [details] [diff] [review]
patch

Review of attachment 8966344 [details] [diff] [review]:
-----------------------------------------------------------------

Some quetsions inline, if this is urgent at least they have to be resolved, if not I would love to get this test not use functionality from common.js in mochitests

::: accessible/tests/browser/events/browser_test_textcaret.js
@@ +10,5 @@
> +function caretMoveChecker(target, caretOffset) {
> +  return function(event) {
> +    let cmEvent = null;
> +    try {
> +      cmEvent = event.QueryInterface(nsIAccessibleCaretMoveEvent);

When can this fail? We filter events by the event type first, so you should not get non caret move event here.

@@ +21,5 @@
> +}
> +
> +async function runTests(browser, accDoc) {
> +  let url = "about:mozilla";
> +  let newWin = await BrowserTestUtils.openNewBrowserWindow({ url });

You can still avoid doing this manually and make use of addAccessibleTask. If you need a window just use browser.contentWindow

@@ +24,5 @@
> +  let url = "about:mozilla";
> +  let newWin = await BrowserTestUtils.openNewBrowserWindow({ url });
> +
> +  let urlbarInputEl = newWin.document.getElementById("urlbar").inputField;
> +  let urlbarInput = getAccessible(urlbarInputEl, [ nsIAccessibleText ]);

I'm a little hesitant that we do this like this (e.g. using functionality of common.js from mochitests). In the very least this needs a comment that these are in parent process.

@@ +33,5 @@
> +  ]);
> +
> +  urlbarInput.caretOffset = -1;
> +  await onCaretMove;
> +  ok(true, 'Caret move in URL bar #1');

not necessary, if await fails the test times out.

@@ +35,5 @@
> +  urlbarInput.caretOffset = -1;
> +  await onCaretMove;
> +  ok(true, 'Caret move in URL bar #1');
> +
> +  onCaretMove = waitForEvents([

waitForEvent instead

@@ +41,5 @@
> +  ]);
> +
> +  urlbarInput.caretOffset = 0;
> +  await onCaretMove;
> +  ok(true, 'Caret move in URL bar #2');

This is not necessary, if await never resolves, the test times out.

@@ +43,5 @@
> +  urlbarInput.caretOffset = 0;
> +  await onCaretMove;
> +  ok(true, 'Caret move in URL bar #2');
> +
> +  await BrowserTestUtils.closeWindow(newWin);

This would not be needed if you use what addAccessibleTask provides.

        Attachment #8966344 -
        Flags: review?(yzenevich) → review+

    
    

    
  
Comment on attachment 8966344 [details] [diff] [review]
patch

Review of attachment 8966344 [details] [diff] [review]:
-----------------------------------------------------------------

Some quetsions inline, if this is urgent at least they have to be resolved, if not I would love to get this test not use functionality from common.js in mochitests

::: accessible/tests/browser/events/browser_test_textcaret.js
@@ +10,5 @@
> +function caretMoveChecker(target, caretOffset) {
> +  return function(event) {
> +    let cmEvent = null;
> +    try {
> +      cmEvent = event.QueryInterface(nsIAccessibleCaretMoveEvent);

When can this fail? We filter events by the event type first, so you should not get non caret move event here.

@@ +21,5 @@
> +}
> +
> +async function runTests(browser, accDoc) {
> +  let url = "about:mozilla";
> +  let newWin = await BrowserTestUtils.openNewBrowserWindow({ url });

You can still avoid doing this manually and make use of addAccessibleTask. If you need a window just use browser.contentWindow

@@ +22,5 @@
> +
> +async function runTests(browser, accDoc) {
> +  let url = "about:mozilla";
> +  let newWin = await BrowserTestUtils.openNewBrowserWindow({ url });
> +

Ah I realize now that this is because it opens window not tab, do you think you could extend our addAccessibleTask to optionally open new window and not tab?

@@ +24,5 @@
> +  let url = "about:mozilla";
> +  let newWin = await BrowserTestUtils.openNewBrowserWindow({ url });
> +
> +  let urlbarInputEl = newWin.document.getElementById("urlbar").inputField;
> +  let urlbarInput = getAccessible(urlbarInputEl, [ nsIAccessibleText ]);

I'm a little hesitant that we do this like this (e.g. using functionality of common.js from mochitests). In the very least this needs a comment that these are in parent process.

@@ +33,5 @@
> +  ]);
> +
> +  urlbarInput.caretOffset = -1;
> +  await onCaretMove;
> +  ok(true, 'Caret move in URL bar #1');

not necessary, if await fails the test times out.

@@ +35,5 @@
> +  urlbarInput.caretOffset = -1;
> +  await onCaretMove;
> +  ok(true, 'Caret move in URL bar #1');
> +
> +  onCaretMove = waitForEvents([

waitForEvent instead

@@ +41,5 @@
> +  ]);
> +
> +  urlbarInput.caretOffset = 0;
> +  await onCaretMove;
> +  ok(true, 'Caret move in URL bar #2');

This is not necessary, if await never resolves, the test times out.

@@ +43,5 @@
> +  urlbarInput.caretOffset = 0;
> +  await onCaretMove;
> +  ok(true, 'Caret move in URL bar #2');
> +
> +  await BrowserTestUtils.closeWindow(newWin);

This would not be needed if you use what addAccessibleTask provides.

    
    

    
  
urgh, i only meant to add this comment:

@@ +22,5 @@
> +
> +async function runTests(browser, accDoc) {
> +  let url = "about:mozilla";
> +  let newWin = await BrowserTestUtils.openNewBrowserWindow({ url });
> +

Ah I realize now that this is because it opens window not tab, do you think you could extend our addAccessibleTask to optionally open new window and not tab?

    
    

    
  
(In reply to :Gijs (he/him) from comment #16)
> Comment on attachment 8966344 [details] [diff] [review]
> patch
> 
> Review of attachment 8966344 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> FWIW, there seems to be no patch header here with a commit message, and also
> there's almost no context on the diffs. It would be nice to fix this in your
> setup, because reviewing e.g. the BrowserTestUtils change is quite painful
> like this - there's no context to see what you're doing.

I was believing I have standard setup :) hg qdiff produces exactly what it produces. I definitely can commit message, however how context stuff work exactly?

BrowserTestUtils change is to make a new window open with a given URL. Apparently, it is two different things for this particular test a) when you open a window and then load URL and b) when you open a window with a given URL.

> Tentative r=me but please do fix the issues below, especially the skip-if,
> after which you may want a new tryrun if you've done one already.
> 
> ::: accessible/tests/browser/events/browser.ini
> @@ +10,1 @@
> >  skip-if = e10s
> 
> You're not running this test almost anywhere now, because it's skip-if=e10s,
> whereas the docload test is no longer skipped. I assume you want to insert
> it 1 line down.

thanks, the ignore syntax confused me, will fix it

> 
> ::: accessible/tests/browser/events/browser_test_textcaret.js
> @@ +1,3 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> Nit: the original test was not licensed, and we normally license tests PD.
> Example header:
> 
> 
> /* Any copyright is dedicated to the Public Domain.
>  * http://creativecommons.org/publicdomain/zero/1.0/ */

k

> > +async function runTests(browser, accDoc) {
> 
> Both of these arguments seem unused. Could you just use a normal add_task()
> ? Is there anything specific you need from the helper?

you're right

> Unrelated, nit: please name the task something more descriptive than
> 'runTests', maybe something like 'checkURLBarCaretEvents' or whatever

fine with me

    
    

    
  
(In reply to Yura Zenevich [:yzen] from comment #19)

> Some quetsions inline, if this is urgent at least they have to be resolved,
> if not I would love to get this test not use functionality from common.js in
> mochitests

can you elaborate this please?

> ::: accessible/tests/browser/events/browser_test_textcaret.js
> @@ +10,5 @@
> > +function caretMoveChecker(target, caretOffset) {
> > +  return function(event) {
> > +    let cmEvent = null;
> > +    try {
> > +      cmEvent = event.QueryInterface(nsIAccessibleCaretMoveEvent);
> 
> When can this fail? We filter events by the event type first, so you should
> not get non caret move event here.

you're right, it shouldn't fail, and if it does, then it's better to not hide it.

> > +async function runTests(browser, accDoc) {
> > +  let url = "about:mozilla";
> > +  let newWin = await BrowserTestUtils.openNewBrowserWindow({ url });
> 
> You can still avoid doing this manually and make use of addAccessibleTask.
> If you need a window just use browser.contentWindow

addAccessibleTask opens a new tab via BrowserTestUtils.withNewTab, while the original test was opening a new window to test Firefox UI. This is a simple mochitest conversion, and I didn't want to make any logical changes.

> > +async function runTests(browser, accDoc) {
> > +  let url = "about:mozilla";
> > +  let newWin = await BrowserTestUtils.openNewBrowserWindow({ url });
> > +
> 
> Ah I realize now that this is because it opens window not tab, do you think
> you could extend our addAccessibleTask to optionally open new window and not
> tab?

right :) the function will become a bit more complicated, so I think I like simplicity of the taken approach.


> > +  let urlbarInputEl = newWin.document.getElementById("urlbar").inputField;
> > +  let urlbarInput = getAccessible(urlbarInputEl, [ nsIAccessibleText ]);
> 
> I'm a little hesitant that we do this like this (e.g. using functionality of
> common.js from mochitests). In the very least this needs a comment that
> these are in parent process.

So you are about getAccessible? What is alternative?

> 
> @@ +33,5 @@
> > +  ]);
> > +
> > +  urlbarInput.caretOffset = -1;
> > +  await onCaretMove;
> > +  ok(true, 'Caret move in URL bar #1');
> 
> not necessary, if await fails the test times out.

correct, but if it passes, it complains the test doesn't have any oks or todos, which makes it failing locally.

(In reply to Yura Zenevich [:yzen] from comment #17)
----------------------------------------------------
> Some eslint errors:
> 
> ...browser_test_textcaret.js
>   20:4   error  Missing semicolon.             semi (eslint)
>   37:12  error  Strings must use doublequote.  quotes (eslint)
>   40:65  error  Unexpected trailing comma.     comma-dangle (eslint)
>   45:12  error  Strings must use doublequote.  quotes (eslint)

yeah, filed the patch on review before starting try builds, for some reason, no complains when it's run locally

    
    

    
  
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/090f4e5a8d73
convert mochitest/textcaret/test_browserui.xul test into a browser test, r=yzen, gijs

    
    

    
  
(In reply to Pulsebot from comment #23)
> Pushed by surkov.alexander@gmail.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/090f4e5a8d73
> convert mochitest/textcaret/test_browserui.xul test into a browser test,
> r=yzen, gijs

landed to stop bleeding, please let me know I need to file any follows

    
  
Keywords: leave-open

    
    

    
  
What is TV tests are about? Here's relevant part of a log:

[task 2018-04-10T18:25:36.687Z] 18:25:36     INFO - *** Start BrowserChrome Test Results ***
[task 2018-04-10T18:25:36.728Z] 18:25:36     INFO - checking window state
[task 2018-04-10T18:25:36.798Z] 18:25:36     INFO - TEST-START | accessible/tests/browser/events/browser_test_textcaret.js
[task 2018-04-10T18:25:37.517Z] 18:25:37     INFO - GECKO(1052) | [Parent 1052, Main Thread] WARNING: g_type_add_interface_static: assertion 'g_type_parent (interface_type) == G_TYPE_INTERFACE' failed: 'glib warning', file /builds/worker/workspace/build/src/toolkit/xre/nsSigHandlers.cpp, line 141
[task 2018-04-10T18:25:37.519Z] 18:25:37     INFO - GECKO(1052) | (firefox:1052): GLib-GObject-CRITICAL **: g_type_add_interface_static: assertion 'g_type_parent (interface_type) == G_TYPE_INTERFACE' failed
[task 2018-04-10T18:25:37.519Z] 18:25:37     INFO - GECKO(1052) | [Parent 1052, Main Thread] WARNING: g_type_add_interface_static: assertion 'g_type_parent (interface_type) == G_TYPE_INTERFACE' failed: 'glib warning', file /builds/worker/workspace/build/src/toolkit/xre/nsSigHandlers.cpp, line 141
[task 2018-04-10T18:25:37.520Z] 18:25:37     INFO - GECKO(1052) | (firefox:1052): GLib-GObject-CRITICAL **: g_type_add_interface_static: assertion 'g_type_parent (interface_type) == G_TYPE_INTERFACE' failed
[task 2018-04-10T18:25:38.110Z] 18:25:38     INFO - GECKO(1052) | MEMORY STAT vsizeMaxContiguous not supported in this build configuration.
[task 2018-04-10T18:25:38.111Z] 18:25:38     INFO - GECKO(1052) | MEMORY STAT | vsize 2066MB | residentFast 293MB | heapAllocated 114MB
[task 2018-04-10T18:25:38.111Z] 18:25:38     INFO - TEST-OK | accessible/tests/browser/events/browser_test_textcaret.js | took 1315ms
[task 2018-04-10T18:25:38.232Z] 18:25:38     INFO - checking window state
[task 2018-04-10T18:25:38.252Z] 18:25:38     INFO - TEST-START | accessible/tests/browser/events/browser_test_textcaret.js
[task 2018-04-10T18:26:23.277Z] 18:26:23     INFO - TEST-INFO | started process screentopng
[task 2018-04-10T18:26:23.846Z] 18:26:23     INFO - TEST-INFO | screentopng: exit 0
[task 2018-04-10T18:26:23.848Z] 18:26:23     INFO - Buffered messages logged at 18:25:38
[task 2018-04-10T18:26:23.849Z] 18:26:23     INFO - Entering test bound checkURLBarCaretEvents
[task 2018-04-10T18:26:23.850Z] 18:26:23     INFO - Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 170}]
[task 2018-04-10T18:26:23.851Z] 18:26:23     INFO - Buffered messages finished
[task 2018-04-10T18:26:23.853Z] 18:26:23     INFO - TEST-UNEXPECTED-FAIL | accessible/tests/browser/events/browser_test_textcaret.js | Test timed out - 
[task 2018-04-10T18:26:23.854Z] 18:26:23     INFO - GECKO(1052) | MEMORY STAT | vsize 2205MB | residentFast 301MB | heapAllocated 104MB
[task 2018-04-10T18:26:23.855Z] 18:26:23     INFO - TEST-OK | accessible/tests/browser/events/browser_test_textcaret.js | took 45040ms
[task 2018-04-10T18:26:23.856Z] 18:26:23     INFO - checking window state
[task 2018-04-10T18:26:23.857Z] 18:26:23     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-04-10T18:26:23.858Z] 18:26:23     INFO - TEST-UNEXPECTED-FAIL | accessible/tests/browser/events/browser_test_textcaret.js | Found a browser window after previous test timed out - 

Yura, any ideas what is might be or how to investigate it further? Local runs + a11y try server runs are all green.
Flags: needinfo?(surkov.alexander) → needinfo?(yzenevich)

    
    

    
  
TV == test-verify.  We find any changed tests in the commits and run them many times (by themselves in a browser, repeated 10 times inside a browser, etc.)

The goal is to find tests that could be intermittent or influencing other tests by changing the browser state, etc.

    
    

    
  
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #27)
> TV == test-verify.  We find any changed tests in the commits and run them
> many times (by themselves in a browser, repeated 10 times inside a browser,
> etc.)

got it, thanks, can I run those locally?

    
    

    
  
:gbrown- do you know if there is a way to do a |./mach test-verify| or something similar?
Flags: needinfo?(gbrown)

    
    

    
  
General info on test-verify at https://developer.mozilla.org/en-US/docs/Mozilla/QA/Test_Verification

To run locally, just add --verify to your mach command line:

  mach mochitest accessible/tests/mochitest/textcaret/test_browserui.xul --verify

To run TV on try, just include a patch that modifies test_browserui.xul in your push and run something like

./mach try ... -u test-verify-e10s,test-verify
Flags: needinfo?(gbrown)

    
    

    
  
By the way, TV is tier 2 and should not result in backout. In this case, I think I saw normal M-a11y intermittent failures start quickly as well, so it might be best to just make the test run reliably anyway.

    
    

    
      
  

    
    

    
  
(In reply to alexander :surkov from comment #21)
> (In reply to :Gijs (he/him) from comment #16)
> > Comment on attachment 8966344 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 8966344 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > FWIW, there seems to be no patch header here with a commit message, and also
> > there's almost no context on the diffs. It would be nice to fix this in your
> > setup, because reviewing e.g. the BrowserTestUtils change is quite painful
> > like this - there's no context to see what you're doing.
> 
> I was believing I have standard setup :) hg qdiff produces exactly what it
> produces. I definitely can commit message, however how context stuff work
> exactly?

My [diff] section in hgrc is:

[diff]
git = 1
showfunc = 1
unified = 10

which fixes this at least for `hg diff` and provides 10 lines of context each side - I don't know about `hg qdiff`, as I stopped using mq a long time ago. To get a patch header you can use `hg bzexport` on a given commit to put it on bugzilla directly (probably requires some setup), or manually `hg export` to a file and then attach that file, rather than just using `diff` which doesn't include the commit message etc.

    
    

    
  
Hey Alex, I dug a bit deeper into this, it fails TV because we do not wait for doc load event on the new window before carrying on with the tests:

I managed to run the test with --verify flag successfully after the following changes:

From this:

let url = "about:mozilla";
let newWin = await BrowserTestUtils.openNewBrowserWindow({ url });

To this:

let url = "about:mozilla";
let docLoaded = waitForEvent(EVENT_DOCUMENT_LOAD_COMPLETE, event =>
  event.accessible.name.includes("The Book of Mozilla"));
let newWin = await BrowserTestUtils.openNewBrowserWindow({ url });
await docLoaded;

I don't really like the match on name, but I could not think of anything prettier (e.g. the dom node), since the window is not available before openNewBrowserWindow completes and there might be a race with starting to listen for the doc load event after that.
Flags: needinfo?(yzenevich)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch2
        — 
          — Splinter Review
      

    


  
with Yura's idea incorporated

        Attachment #8966344 -
        Attachment is obsolete: true

    
  
Keywords: checkin-needed

    
    

    
  
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11a9cb8e8200
Convert mochitest/textcaret/test_browserui.xul test into a browser test. r=yzen, gijs
Keywords: checkin-needed

    
    

    
      
  

    
    

    
  
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/725ba91eecc1
convert mochitest/textcaret/test_browserui.xul test into a browser test, r=yzen, gijs

    
    

    
  
(In reply to Pulsebot from comment #39)
> Pushed by surkov.alexander@gmail.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/725ba91eecc1
> convert mochitest/textcaret/test_browserui.xul test into a browser test,
> r=yzen, gijs

this one should work
Flags: needinfo?(surkov.alexander)

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/725ba91eecc1
Status: NEW → RESOLVED
Closed: 6 years ago

          status-firefox61:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Whiteboard: [stockwell disable-recommended] → [stockwell fixed:other]

    
    

    
      
  

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