The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bcfa6fa04a51c021afa74ca599e9c488f7a89ed9

commit bcfa6fa04a51c021afa74ca599e9c488f7a89ed9
Author: johnme <johnme@chromium.org>
Date: Fri Aug 26 17:52:43 2016

Mark SettingsLanguagesPageBrowserTest.LanguagesPage flaky on Windows

BUG= 641400 
TBR=michaelpg@chromium.org
NOTRY=true
NOTREECHECKS=true

Review-Url: https://codereview.chromium.org/2284823002
Cr-Commit-Position: refs/heads/master@{#414743}

[modify] https://crrev.com/bcfa6fa04a51c021afa74ca599e9c488f7a89ed9/chrome/test/data/webui/settings/languages_page_browsertest.js

SettingsLanguagesPageBrowserTest.LanguagesPage failed on Mac10.10 Tests in https://build.chromium.org/p/chromium.mac/builders/Mac10.10%20Tests/builds/13482

 Issue 711644  has been merged into this issue.

Detected 3 new flakes for test/step "SettingsLanguagesPageBrowserTest.LanguagesPage". To see the actual flakes, please visit https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyOQsSBUZsYWtlIi5TZXR0aW5nc0xhbmd1YWdlc1BhZ2VCcm93c2VyVGVzdC5MYW5ndWFnZXNQYWdlDA. This message was posted automatically by the chromium-try-flakes app. Since flakiness is ongoing, the issue was moved back into Sheriff Bug Queue (unless already there).

We're seeing flake on Linux and Mac as well.

Disabling this test in general.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/92bf225e9075f1b95dda6cbd288c1d8729ee97f1

commit 92bf225e9075f1b95dda6cbd288c1d8729ee97f1
Author: rogerm <rogerm@chromium.org>
Date: Wed Apr 19 20:01:09 2017

Disable SettingsLanguagesPageBrowserTest.LanguagesPage

Flakiness has expanded from just Windows and memory bots to Mac and
Linux as well.

Broadly disabling the test.

BUG= 641400 
TBR=michaelpg@chromium.org

Review-Url: https://codereview.chromium.org/2828063002
Cr-Commit-Position: refs/heads/master@{#465721}

[modify] https://crrev.com/92bf225e9075f1b95dda6cbd288c1d8729ee97f1/chrome/test/data/webui/settings/languages_page_browsertest.js

Michael, are you still the right owner for this? If not, can you send along to whoever owns Settings now?

reassigning to dpapad who is actively working on these tests the past few days

I am able to reproduce the flakiness locally with --gtest_repeat=5

AssertionError: expected <settings-add-languages-dialog>
      </settings-add-languages-dialog> to equal null
    at Function.assert.strictEqual (chai.js:2277:32)
    at assertEquals (test_api.js:893:17)
    at Context.<anonymous> (languages_page_browsertest.js:152:9)
", source: mocha_adapter.js (48)

So after investigating, I believe these tests can be deflaked, but it requires refactoring significantly. Specifically, currently they work as follows:

 1) SettingsLanguagesPageBrowserTest loads the entire chrome://md-settings page, which is unnecessary, only languages_page.html should be loaded, and a settings-languages-page element should be created on demand.
 2) Then it toggles the "advanced" toggle, hides all sections on chrome://md-settings and un-hides the "languages" section, see [1]. 
 3) Finally the code does not use any "fakes" for interacting with the browser, instead it is hitting the actual C++ APIs during testing.

@michaelpg: I would like to ensure that you are OK with such a refactoring before spending more time on this. Specifically what I have in mind is the following:
 1) Change SettingsLanguagesPageBrowserTest to inherit from CrSettingsBrowserTest [2] like most of our tests, instead of SettingsPageBrowserTest.
 2) Move the C++ fixture in cr_settings_browsertests.js. Move the actual Mocha tests to a new languages_page_tests.js (similar to how languages_tests.js).
 3) Change the "browserPreload" URL to languages_page.html
 4) Do necessary refactoring to reuse the setup code (fake prefs + languageHelper + TestLanguagesBrowserProxy) across similar tests.
 5) Programmatically create a <settings-languages-page> and set it up such that it uses fake versions of all APIs involved.

[1] https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/languages_page_browsertest.js?l=30,48,87
[2] https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/cr_settings_browsertest.js?l=19

Let me know WDYT? Also if you prefer to take upon this refactoring yourself, that would be very helpful too.

I would just want some more detail on how the flakes occur, in terms of whether we'd need to change all three steps in the first list. I'm happy to handle the test refactoring however we decide to do it, so you guys can focus on launch stuff.

1), 2) and 3) all stem from what the team wanted at the time -- a mix of unit tests (e.g., of the model -- languages_tests.js) and "integration" style SettingsPageBrowserTests, especially when the test involved opening/closing subpages or dialogs. +stevenjb as he also wanted this, IIRC. If the team has decided that's dangerous for some reason, like... if the old way causes too many flakes to justify it ;-) then just be sure to broadcast that so we know for future tests or refactors.

Re 1) and 2): Some other tests we may want to change: https://cs.chromium.org/search/?q=settingspagebrowsertest+-file:settings/(settings%7Cbasic%7Cadvanced)_page_browsertest+-file:(settings_subpage%7Ccr_settings)_browsertest&type=cs

Re 3): We could use fakes here, at the cost of significant test coverage -- nothing's testing the JS extension bindings or the chrome.send handlers, so they could get out of sync or stop working for edge cases. I'd want something testing that. (Maybe test that languages_page.html calls the mocks we expect, and we add a C++ unit test for the APIs we use. WDYT? or am I just being paranoid about the level of tests necessary?)

I think it's common for browser_tests to test the real results of using the browser. But tests that work are better than disabled tests, so I see why your suggestion is good. I'd still like to dig a little deeper into why loading the page is triggering these flakes -- I'm willing to take that on too. If it's already obvious to you guys why the flakes happen, sorry if I'm not in the loop, just let me know.

broadcasting "the old way causes too many flakes to justify it" so you know for future tests or refactors

#16: thank you for that terse and pointed reply using my words. makes me feel great about carefully editing my comment to try my best to keep it helpful, understanding, and constructive, to show the team I'm not just trying to be difficult and value their collective input more than my own.

> I'd still like to dig a little deeper into why loading the page is triggering these flakes -- I'm willing to take that on too. If it's already obvious to you guys why the flakes happen, sorry if I'm not in the loop, just let me know.

It is not 100% clear to me why the test is flaking. See comment#12 above with the error I was able to reproduce on Linux, where the dialog is not closed when the test assumes it should be).

I investigated this for a while, but after encountering some nuances like nested mocha suite() calls, setTimeout() calls, as well as unit-testing anti-patterns (your explanation that these were designed as integration tests makes complete sense now) I gave up. If you can locate a smaller fix for the flake than converting the test from an integration test to a unit test, I am OK with it.

Overall, I do think that the "SettingsPageBrowserTest" subclasses you linked represent an older style of testing in our codebase. It would probably be beneficial if we can decide whether we can live with unit-tests only, and if so convert all of these accordingly. 

dpapad: OK. I'll get started on the test refactoring. Setting owner back to me.

Thanks for picking this up!

Busy gardening this week. Will update CL on Monday: https://codereview.chromium.org/2829383002/

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dbea67d82a6fc4caa4558ae3066bfa76afdc18e1

commit dbea67d82a6fc4caa4558ae3066bfa76afdc18e1
Author: michaelpg <michaelpg@chromium.org>
Date: Fri May 05 22:54:58 2017

Change SettingsLanguagesPageBrowserTest to CrSettingsLanguagesPageTest

Transforms a browser test to a lighterweight browser test in an attempt
to avoid test flakes: don't load the settings page (in non-vulcanized
builds), don't make round trips to the real browser settings, and split
up the tests into four individual browser tests.

SettingsLanguagesPageBrowserTest is currently a SettingsPageBrowserTest
test fixture that loads chrome://md-settings with real browser settings
to test language settings.

This CL changes it into a CrSettingsBrowserTest, which differs by
loading only the <settings-languages-page> file and its dependencies,
starting with a blank page, mocking the various settings APIs, and
testing the element in isolation.

On a vulcanized build, we still load the entire chrome://md-settings
page. Once it loads, though, we clear that page, mock the various
settings APIs, and can still test a <settings-languages-page> in
relative isolation.

The mocha test suite is invoked in four parts, so each browser test has
to execute fewer mocha tests and should finish in less time.

R=dpapad@chromium.org
BUG= 641400 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2837703002
Cr-Commit-Position: refs/heads/master@{#469789}

[modify] https://crrev.com/dbea67d82a6fc4caa4558ae3066bfa76afdc18e1/chrome/browser/resources/settings/languages_page/languages_page.html
[modify] https://crrev.com/dbea67d82a6fc4caa4558ae3066bfa76afdc18e1/chrome/test/data/webui/BUILD.gn
[modify] https://crrev.com/dbea67d82a6fc4caa4558ae3066bfa76afdc18e1/chrome/test/data/webui/settings/cr_settings_browsertest.js
[modify] https://crrev.com/dbea67d82a6fc4caa4558ae3066bfa76afdc18e1/chrome/test/data/webui/settings/fake_language_settings_private.js
[delete] https://crrev.com/3caa6e14cb856d09bc62c2bbbcae32e2aeadf95b/chrome/test/data/webui/settings/languages_page_browsertest.js
[add] https://crrev.com/dbea67d82a6fc4caa4558ae3066bfa76afdc18e1/chrome/test/data/webui/settings/languages_page_tests.js
[modify] https://crrev.com/dbea67d82a6fc4caa4558ae3066bfa76afdc18e1/chrome/test/data/webui/settings/languages_tests.js
[add] https://crrev.com/dbea67d82a6fc4caa4558ae3066bfa76afdc18e1/chrome/test/data/webui/settings/test_languages_browser_proxy.js
[modify] https://crrev.com/dbea67d82a6fc4caa4558ae3066bfa76afdc18e1/chrome/test/data/webui/settings/test_util.js
[modify] https://crrev.com/dbea67d82a6fc4caa4558ae3066bfa76afdc18e1/ui/webui/resources/cr_elements/shared_vars_css.html

We still have a few flakes, though they have all passed after retry/ies.

Flakiness dashboard: https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=CrSettingsLanguagesPageTest

On Win: CrSettingsLanguagesPageTest.LanguageMenu/SpellCheck occasionally time out -- only on FYI bots

On Mac: CrSettingsLanguagesPageTest.AddLanguagesDialog occasionally fails. Log follows:

[ RUN      ] CrSettingsLanguagesPageTest.AddLanguagesDialog
[75344:14851:0508/011926.623983:WARNING:notification_platform_bridge_mac.mm(493)] AlertNotificationService: XPC connection invalidated.
[75344:771:0508/011931.925873:INFO:CONSOLE(1221)] "Running TestCase CrSettingsLanguagesPageTest.AddLanguagesDialog", source: test_api.js (1221)
[75344:771:0508/011932.673183:WARNING:CONSOLE(197)] "/deep/ combinator is deprecated and will be a no-op in M60, around August 2017. See https://www.chromestatus.com/features/4964279606312960 for more details.", source: polymer_browser_test_base.js (197)
[75344:771:0508/011933.325244:ERROR:CONSOLE(48)] "Mocha test failed: languages page add languages dialog "after each" hook for "cancel"
AssertionError: expected <settings-add-languages-dialog>
      </settings-add-languages-dialog> to equal null
    at Function.assert.strictEqual (chai.js:2277:32)
    at assertEquals (test_api.js:893:17)
    at Context.<anonymous> (languages_page_tests.js:113:9)
", source: mocha_adapter.js (48)
[75344:771:0508/011933.755371:INFO:CONSOLE(125)] "Page load time: 2192 ms", source: polymer_browser_test_base.js (125)
[75344:771:0508/011933.755556:INFO:CONSOLE(127)] "Test run time: 1822 ms", source: polymer_browser_test_base.js (127)
[75344:771:0508/011933.755649:INFO:CONSOLE(129)] "Total time: 4014 ms", source: polymer_browser_test_base.js (129)
[75344:771:0508/011933.755794:ERROR:web_ui_test_handler.cc(76)] Test Errors: 1/2 tests had failed assertions.
[75344:771:0508/011933.756252:ERROR:web_ui_browser_test.cc(467)] CONDITION FAILURE: encountered javascript console error(s):
[75344:771:0508/011933.756288:ERROR:web_ui_browser_test.cc(469)] JS ERROR: '[75344:771:0508/011933.325244:ERROR:CONSOLE(48)] "Mocha test failed: languages page add languages dialog "after each" hook for "cancel"
AssertionError: expected <settings-add-languages-dialog>
      </settings-add-languages-dialog> to equal null
    at Function.assert.strictEqual (chai.js:2277:32)
    at assertEquals (test_api.js:893:17)
    at Context.<anonymous> (languages_page_tests.js:113:9)
", source: mocha_adapter.js (48)
'
[75344:771:0508/011933.756313:ERROR:web_ui_browser_test.cc(471)] JS call assumed failed, because JS console error(s) found.
gen/chrome/test/data/webui/settings/cr_settings_browsertest-gen.cc:2006: Failure
Value of: RunJavascriptTestF( true, "CrSettingsLanguagesPageTest", "AddLanguagesDialog")
  Actual: false
Expected: true
[  FAILED  ] CrSettingsLanguagesPageTest.AddLanguagesDialog, where TypeParam =  and GetParam() =  (7831 ms)
[465/618] CrSettingsLanguagesPageTest.AddLanguagesDialog (8465 ms)

I can reproduce the issue locally: the contents of the <template is="dom-if" restamp> is, sometimes, not removed from the DOM after async + setTimeout.

Using Polymer.dom.flush() followed by setTimeout seems to work but I'll try it on the bots before updating the test.

Crrraaaaaaapppp debugging this has not been fun.

First, let's consider a test that taps the Cancel button and verifies that the showAddLanguagesDialog_ boolean is set
to false.

*This test will always fail.*

1. Clicking the cancel button synchronously calls HTMLDialogElement::close()
2. WebKit QUEUES a "close" event -- it does not immediately dispatch it!
3. Polymer.dom.flush() doesn't do anything: the close() event hasn't happened!
4. The close event listener updates Polymer's showAddLanguagesDialog_ boolean, so if we test it now, the test fails.



But wait.... our test sometimes PASSES! Why??

Because our test actually taps the Cancel button in test(), and verifies the dialog is closed in teardown().

So? Well, apparently, mocha does some shenanigans here that, somehow, results in the teardown hook running
ASYNCHRONOUSLY. I did not know this.

This means that sometimes the queued "close" event is dispatched before teardown() runs.
Our test will pass if that happens.



How do we guarantee the test will pass?

setTimeout, you say? WebKit's "close" event is queued on the Document's ScriptedAnimationController's event queue.
That's implementation-dependent.

setTimeout() schedules a task on a *different* event queue (but the code is really hard to decipher from here).

So if we call setTimeout() after clicking the cancel button, then one of those event queues is processed first.
Indeterministically.
 *  If the "close" event is dispatched first:
      UI catches the "close" event. The dom-if's |if| becomes false. 
      Test probably passes, maybe needing a Polymer.dom.flush().
 *  If the setTimeout() callback is called first, well, we're back where we started.

Recursively calling setTimeout -- even specifying a number of milliseconds -- still yields flaky results, as the order
of the "close" event and the setTimeout callbacks is not guaranteed because they're on entirely different task queues.

My CL to use setTimeout will thus still be flaky, assuming teardown()'s secret magic doesn't make it pass, which for all
I know it might.



Alright... what about requestAnimationFrame? No way that'd help, right? HAH! Did I mention WebKit queues the "close"
event on our Document's ScriptedAnimationController? Can you guess where the requestAnimationFrame callback is
delivered? Onto a callback queue on the same controller. And requestAnimationFrame also, well, requests that the
ScriptedAnimationController process its various queues.

It happens to do so in a particular order...

1. DispatchEvents()
2. RunTasks()
3. ExecuteCallbacks()

...which means the "close" event is popped off the queue and dispatched before our rAF callback is called.



I will now make 3 claims.

1. If my analysis is correct, then calling requestAnimationFrame() in teardown() before checking the test result will
   guarantee that the test passes, no setTimeout needed. Assuming the test doesn't time out on a slow bot.

2. Using (1) means relying on the "close" event being added to the event queue owned by the same object which handles
   requestAnimationFrame requests, which is insane. We should just wait for the close event, or the dom-if DOM mutation
   event as dpapad suggested, even though that's dependent on how Settings is implemented.

3. Investigations into these flakes do not scale.

can't you just return a Promise that gets resolved with a mutation observer on the dialog's open property?  we do this in other places.

Am I missing something, or is the inherent problem that we are doing an assertion in teardown() ?

My experience with testing is that 'setup' and 'teardown' shouldn't be used for testing.

Can't we just request an animation frame, flush, and test that the dialog is null in the test proper?

I did a quick audit, and languages_page_tests.js appears to be the only test in that directory where we have an assert in teardown().

#28: No, calling close() on an HTMLDialogElement immediately sets the open attribute to false, but the close event fires asynchronously (later).

Observing the actual stamped contents of the dom-if would ensure we react after the close event has fired (plus, that's what we want to test for: the dialog is removed from the DOM)

#29: The fact that teardown() has some odd behavior was only explaining why the test *passed*. Moving the assert to test() would cause it to deterministically fail.

Maybe teardown()'s odd behavior means we should not use it to assert postconditions. That would have prevented this red herring from sneaking up on me/made the test much easier to debug.

Can we use a MutationObserver to detect the removal of the dialog from the DOM? From [1] it seems that MutationObserver can observe additions/removals of certain node's children.

[1] https://davidwalsh.name/mutationobserver-api (search for "removals")

#31: Yes, that works well. https://codereview.chromium.org/2867213002/ (Patch Set 2)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/12ddbd73d25fefda658eb70e10bf94418ee9b51d

commit 12ddbd73d25fefda658eb70e10bf94418ee9b51d
Author: michaelpg <michaelpg@chromium.org>
Date: Thu May 18 05:21:29 2017

Fix CrSettingsLanguagesPageTest dialog flake

BUG= 641400 
R=dpapad@chromium.org

Review-Url: https://codereview.chromium.org/2867213002
Cr-Commit-Position: refs/heads/master@{#472682}

[modify] https://crrev.com/12ddbd73d25fefda658eb70e10bf94418ee9b51d/chrome/test/data/webui/settings/cr_settings_browsertest.js
[modify] https://crrev.com/12ddbd73d25fefda658eb70e10bf94418ee9b51d/chrome/test/data/webui/settings/languages_page_tests.js

Bulk unassigning my language settings bugs as I am no longer working on Settings. 

Hey there.
When I look at https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=CrSettingsLanguagesPageTest I can't see any flake. All tests that run are green ("pass").
So, does that mean that the flakiness is fixed? I would close the bug then.
I am not familiar with these tests, so if somebody who knows them can comment that would be great.

yup, my fix must've stuck :-)