Fix flaky ContextMenu/FilesAppBrowserTest |
||||
Issue descriptionContextMenu/FilesAppBrowserTest were updated in for team drives in [1], and are flakey on the bots, the paste tests in particular. 1) Those tests use two windows and the flake is one of the windows is not found. We should change the paste tests to use one window. 2) Investigating related issue 867974 , we noticed that the queries for the context menu item states could be made more precise, eg., by prefixing them with #file-context-menu:not([hidden]), since they can currently select state from a hidden content menu, leading to a false test positive. 3) Most of the ContextMenu test only use Drive/My Drive/Team Drive, but load Downloads as well in their SetupAndWaitUntilReady() call, but never use it. As we have found in main bug elsewhere, bot speed improves when we load the minimal amount of volume data needed for a FilesApp test. [1] https://chromium-review.googlesource.com/c/chromium/src/+/1104058
,
Aug 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f551fd9e9ac81db8d67dbb31c32aa5eb979a7b76 commit f551fd9e9ac81db8d67dbb31c32aa5eb979a7b76 Author: Noel Gordon <noel@chromium.org> Date: Wed Aug 08 03:35:03 2018 Fix flaky ContextMenu/FilesAppBrowserTest after CL:1104058 Remove the use of two windows for the paste tests: pre-populate system clipboard by copying 'hello.txt' there in the test preamble. Make the query for context menu display + command options more precise by prefixing the menu queries with a non-hidden content menu selector, this to avoid matching command attributes of hidden menus. The 'focus' remote command returns a boolean result: change all places in the test code to assert the 'focus' remote command result. Minor: fix comments where needed. Make tests that only need Drive only load Drive in their setupAndWaitUntilReady() call (for speed). Bug: 871771 Change-Id: I242344b60f405984aaa667f1f4d6c1def97880d0 Reviewed-on: https://chromium-review.googlesource.com/1165270 Reviewed-by: Luciano Pacheco <lucmult@chromium.org> Commit-Queue: Noel Gordon <noel@chromium.org> Cr-Commit-Position: refs/heads/master@{#581462} [modify] https://crrev.com/f551fd9e9ac81db8d67dbb31c32aa5eb979a7b76/ui/file_manager/integration_tests/file_manager/context_menu.js
,
Aug 8
,
Aug 8
and re: this part > 2) Investigating related issue 867974 , we noticed that the queries for the context menu item states could be made more precise, eg., by prefixing them with #file-context-menu:not([hidden]), since they can currently select state from a hidden content menu, leading to a false test positive. Looking over the main.html cr-menus html, this appears to be a general problem now we have so many menus with the same command id names, "paste", "copy", etc Spitting out a general bug for that, ... issue 872119
,
Aug 8
Another thing noticed from CL:1104058 and in fixing issue 867974 , the fakeMouseRightClick code change [1]. Suspect is was not needed, and more a symptom of the test case not focusing the element it wants before sending a mouse click test.util.sync.fakeMouseRightClick = function(contentWindow, targetQuery) { + contentWindow.document.querySelector(targetQuery).focus(); var mouseDownEvent = new MouseEvent('mousedown', {bubbles: true, button: 2}); if (!test.util.sync.sendEvent(contentWindow, targetQuery, mouseDownEvent)) { return false; } Let's see if we can remove that part of the change, since no other test in the suite has needed the focus() call in test.util.sync.fakeMouseRightClick [1] https://chromium-review.googlesource.com/c/chromium/src/+/1104058/10/ui/file_manager/file_manager/background/js/test_util_base.js
,
Aug 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/521a9d2dc738bca089bca4a0f0813e8fc656c977 commit 521a9d2dc738bca089bca4a0f0813e8fc656c977 Author: Noel Gordon <noel@chromium.org> Date: Wed Aug 08 06:34:47 2018 Remove CL:1104058 change to test.util.sync.fakeMouseRightClick CL:1104058 added a focus() call internal to this test helper. No other testcase in the FilesAppBrowserTest suite has ever needed it, and many tests use test.util.sync.fakeMouseRightClick already. Suspecting this additional focus() was a hangover to fix problems with testcases added in CL:1104058 during development. They got fixed later when the testcases added the correct remote calls to focus the element before right-clicking that element, we suppose. Anyhow, litmus test is removing the focus() call, and then running all tests --gtest_filter="ContextMenu*" in RELEASE and DEBUG 5 times back- to-back. Result: no problems, this line of code can away. Test: browser_test --gtest_filter="ContextMenu/FileApps*" Bug: 871771 Change-Id: I20577b7cbda0b709b40ff83ec96e8be4838a36bb Reviewed-on: https://chromium-review.googlesource.com/1166754 Reviewed-by: Luciano Pacheco <lucmult@chromium.org> Commit-Queue: Luciano Pacheco <lucmult@chromium.org> Cr-Commit-Position: refs/heads/master@{#581486} [modify] https://crrev.com/521a9d2dc738bca089bca4a0f0813e8fc656c977/ui/file_manager/file_manager/background/js/test_util_base.js
,
Aug 9
|
||||
►
Sign in to add a comment |
||||
Comment 1 by noel@chromium.org
, Aug 7