updating our infrastructure/tooling to fix bugs/features we have a new worker which is responsible for accepting the tests to run and launching the mozharness/test script to run the tests. The main difference in the new worker is that we login as an actual user instead of simulating it with runas and mapping directories. while doing this we see failures [1]: 22:42:01 INFO - 968 INFO TEST-START | toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_delayedbutton.xul 22:42:03 INFO - Not taking screenshot here: see the one that was previously logged 22:42:03 INFO - Buffered messages logged at 22:42:02 22:42:03 INFO - 969 INFO TEST-PASS | toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_delayedbutton.xul | A valid string reason is expected 22:42:03 INFO - 970 INFO TEST-PASS | toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_delayedbutton.xul | Reason cannot be empty 22:42:03 INFO - 971 INFO TEST-PASS | toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_delayedbutton.xul | button started disabled 22:42:03 INFO - Buffered messages logged at 22:42:03 22:42:03 INFO - 972 INFO TEST-PASS | toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_delayedbutton.xul | button was enabled 22:42:03 INFO - Buffered messages finished 22:42:03 ERROR - 973 INFO TEST-UNEXPECTED-FAIL | toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_delayedbutton.xul | button was disabled - got false, expected true 22:42:03 INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:312:5 22:42:03 INFO - test_aboutCrashed@chrome://mochitests/content/chrome/toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_delayedbutton.xul:83:9 22:42:03 INFO - async*doTest@chrome://mochitests/content/chrome/toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_delayedbutton.xul:66:23 22:42:03 INFO - onload@chrome://mochitests/content/chrome/toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_delayedbutton.xul:1:1 22:42:03 INFO - rval@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:146:17 22:42:03 INFO - EventHandlerNonNull*this.addLoadEvent@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:171:13 22:42:03 INFO - @chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1434:5 22:42:03 INFO - 974 INFO must wait for focus 22:42:03 INFO - Not taking screenshot here: see the one that was previously logged 22:42:03 ERROR - 975 INFO TEST-UNEXPECTED-FAIL | toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_delayedbutton.xul | button remained disabled - got false, expected true here is the last passing statement: https://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_delayedbutton.xul#77 then we appear to not disable the button- could this be a side effect of not focusing on the window/dialog? [1] https://taskcluster-artifacts.net/K6qM_CD-SPaHDJEKttRh2w/0/public/logs/live_backing.log

:paolo, can you help me determine if it is ok to skip this on win10, or if there are more ideas you might have for debugging this?

While this should be rewritten as a browser-chrome test and use mutation observers instead of being time-based, you could try to put "await waitDelay(100);" before the failing statement to see if there is a timing issue for which the button isn't disabled immediately when the window loses focus.

I can reproduce this locally if I do not focus on the window element (i.e. doing something else on my computer while this runs in the background). Doing this the |await waitDelay(100)| didn't help at all. If this were rewritten as a browser-chrome test, do you have a suggestion of a similar test I could use for reference?

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #3) > I can reproduce this locally if I do not focus on the window element (i.e. > doing something else on my computer while this runs in the background). > Doing this the |await waitDelay(100)| didn't help at all. If you interacted in any way with the computer after the automated tests have started, the result wouldn't be significant for understanding how the test runs in automation, in other words you may be seeing a different issue. > If this were rewritten as a browser-chrome test, do you have a suggestion of > a similar test I could use for reference? There are some related browser-chrome tests in uriloader/exthandler/tests/mochitest, but I didn't see any that looked like a good starting point. Unfortunately, the code in this component is quite old.

this seems to work for my scenario- I am doing more testing, but thought I would get the review started.

Comment on attachment 8959259 [details] [diff] [review] move test from chrome->browser-chrome Simply porting this to browser-chrome is a good step if it helps solving the immediate issue without too much effort. For the actual review, can you use "hg mv" so that only the minimal amount of changes needed to the file will show up?

I still have 2 eslint errors here: TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/mozapps/downloads/tests/browser/browser_unknownContentType_delayedbutton.js:12:5 | Expected method shorthand. (object-shorthand) TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/mozapps/downloads/tests/browser/browser_unknownContentType_delayedbutton.js:40:5 | listen for events instead of setTimeout() with arbitrary delay (mozilla/no-arbitrary-setTimeout) if you have suggestions for fixing this, I would be happy to learn more or try more.

Comment on attachment 8959560 [details] [diff] [review] move test from chrome->browser-chrome Review of attachment 8959560 [details] [diff] [review]: ----------------------------------------------------------------- I tried the patch locally, and as far as I can tell the tests didn't actually run as expected. There are a few more changes needed, including the addition to "toolkit/mozapps/downloads/tests/moz.build" that was lost in the latest patch. For the next revision, make sure this command doesn't indicate any failure first, then start a tryserver build before asking for review: ./mach test toolkit/mozapps/downloads/tests/browser/ Given the above, I can see no evidence that moving this to browser-chrome will fix the intermittent, but we can do this regardless, because it is an improvement over the current state and gives us access to more tools to improve the test later. ::: toolkit/mozapps/downloads/tests/browser/browser_unknownContentType_delayedbutton.js @@ +1,1 @@ > +ChromeUtils.import("resource://testing-common/PromiseTestUtils.jsm", this); You actually want... ChromeUtils.import("resource://gre/modules/PromiseUtils.jsm", this); ..and then use PromiseUtils.defer() below. @@ -1,4 @@ > -<?xml version="1.0"?> > -<!-- 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/. --> You need to carry over the license header. Use the official ones from here: https://www.mozilla.org/en-US/MPL/headers/ @@ +2,3 @@ > > +const UCT_URI = "chrome://mozapps/content/downloads/unknownContentType.xul"; > +const LOAD_URI = "http://mochi.test:8888/chrome/toolkit/mozapps/downloads/tests/chrome/unknownContentType_dialog_layout_data.txt"; This URI cannot be loaded from a browser-chrome test. You need to move the support files to the new folder and use this URI instead: "http://mochi.test:8888/browser/toolkit/mozapps/downloads/tests/browser/unknownContentType_dialog_layout_data.txt" Given this, and to avoid unnecessary duplication, what I suggest is to move (with Mercurial) all the files in the "chrome" folder to the "browser" folder, including the support and build files, and make the necessary edits to both tests. @@ +11,2 @@ > > + observe: function(aSubject, aTopic, aData) { So, here ESLint simply requires: observe(aSubject, aTopic, aData) { For this test, you can also disable the other rule for the moment with: /* eslint-disable mozilla/no-arbitrary-setTimeout */ @@ +43,4 @@ > > +async function doTest() { > + let frame = document.getElementById("testframe"); > + frame.setAttribute("src", LOAD_URI); The browser-chrome tests do require different test definitions, and since "testframe" doesn't exist the download should be started differently. Use this here instead: add_task(async function test_unknownContentType_delayedbutton() { await BrowserTestUtils.withNewTab({ gBrowser, url: LOAD_URI, }, async function() { The waitForExplicitFinish call above has to be removed. Since you're changing indentation anyways, it would be nice if you could use two spaces instead of four throughout the files. Thanks for helping out here!

thanks for the feedback, this is looking good and I believe these are now testing what they were before. They run clean in the test-verify job on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9008fb024fc34eca206c4b99ae67ff82dacdeb8 and locally it runs without failure. I have cleaned up the eslint errors since the test-verify run. This feels more brute force as I am copying a lot of logic in the layout test, if you have suggestions- do suggest!

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #9) > This feels more brute force as I am copying a lot of logic in the layout > test, if you have suggestions- do suggest! You can enclose the whole body of the asynchronous test function inside a "for (let test of tests)". This should avoid some duplication while keeping the structure of the test similar. Of course, this still allows you to remove all that unnecessary test running infrastructure!

Comment on attachment 8961010 [details] [diff] [review] move chrome/* to browser/* The rest looks good, next round will probably be a quick review.

the for loop worked great, looks good on try as well.

Comment on attachment 8961131 [details] [diff] [review] move chrome/* to browser/* Review of attachment 8961131 [details] [diff] [review]: ----------------------------------------------------------------- Thanks a lot for doing the conversion! ::: toolkit/mozapps/downloads/tests/browser/browser_unknownContentType_dialog_layout.js @@ -4,5 @@ > - - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > -<!-- > - * The unknownContentType popup can have two different layouts depending on > - * whether a helper application can be selected or not. > - * This tests that both layouts have correct collapsed elements. The comment in this test still seems relevant, so it can be carried over. (The one in the other test was a cut and paste leftover.) @@ +11,5 @@ > basicBox: { collapsed: false }, > normalBox: { collapsed: true } > } > }, > + { // This URL will trigger the simple UI, where only the Save an Cancel buttons are available Looks like this comment got changed accidentally.

Comment on attachment 8961131 [details] [diff] [review] move chrome/* to browser/* Review of attachment 8961131 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/downloads/tests/browser/browser_unknownContentType_dialog_layout.js @@ +21,5 @@ > } > ]; > > +for (let test of tests) { > + add_task(async function test_unknownContentType_dialog_layout1() { nit: in other tests in the tree we have the "for" inside the function, it doesn't make a big difference but I suggest you use the same style here. Also the "1" at the end of the function name can be removed.

Pushed by jmaher@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7d110c91eac6 migrate test_unknownContentType_delayedbutton.xul to browser-chrome. r=paolo