New issue
Advanced search Search tips

Issue 871771 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 9
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug

Blocking:
issue 836254



Sign in to add a comment

Fix flaky ContextMenu/FilesAppBrowserTest

Project Member Reported by noel@chromium.org, Aug 7

Issue description

ContextMenu/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
 
Blocking: 836254
Project Member

Comment 2 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
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 
Status: Started (was: Fixed)
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

Project Member

Comment 6 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)

Sign in to add a comment