New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 851888 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug

Blocking:
issue 836254



Sign in to add a comment

Speed-up the FilesApp open/close Quick View browser tests

Project Member Reported by noel@chromium.org, Jun 12 2018

Issue description

The tests test opening and opening+closing the quick view when a the space key is pressed on a selected file.  The selected file is a large PNG image that is decoded and rescaled during these tests to display the result in quick view.

Might be faster to select a simpler file (like a txt file) in these tests, since they only seem to test whether Files App can open / close the Quick View.

 

Comment 1 by noel@chromium.org, Jun 12 2018

Cc: sashab@chromium.org
Status: Started (was: Available)

Comment 2 by noel@chromium.org, Jun 12 2018

Blocking: 836254
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 13 2018

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

commit 438e9f0679fa1ac6889991714d9aa4dff1034d6a
Author: Noel Gordon <noel@chromium.org>
Date: Wed Jun 13 02:51:12 2018

Speed-up QuickView openQuickView browser test

The test tests that Quick View opens by the usual method: user presses
the space key on a selected file.

Change the test to use a text file, rather than a PNG image, to remove
image decode and rescale costs from the test-time.

Also put all tests steps in a StepRunner group, so that all test steps
can be debugged using the step runner browser test flag. Document each
of the steps.

Bug:  851888 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Ief21de93e9496f463506d2ae22ab28db76c087ef
Reviewed-on: https://chromium-review.googlesource.com/1097041
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566699}
[modify] https://crrev.com/438e9f0679fa1ac6889991714d9aa4dff1034d6a/ui/file_manager/integration_tests/file_manager/quick_view.js

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 13 2018

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

commit c0aa77df4da9f1eb1f788a4f04b0cb078e2903ef
Author: Noel Gordon <noel@chromium.org>
Date: Wed Jun 13 02:55:44 2018

Speed-up QuickView closeQuickView browser test

The test tests that Quick View closes when users click Quick View when
it is in open state.

Change the test to use a text file, rather than a PNG image, to remove
image decode and rescale costs from the test-time.

The test awaits the close using a pending() call, but did not define a
|caller|, which is a required argument of pending(). Fix that.

Also put all tests steps in a StepRunner group, so that all test steps
can be debugged using the step runner browser test flag. Document each
of the test steps.

Bug:  851888 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I39310b57f9ed09e5d164b95b11b7494dc45adffa
Reviewed-on: https://chromium-review.googlesource.com/1097048
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566700}
[modify] https://crrev.com/c0aa77df4da9f1eb1f788a4f04b0cb078e2903ef/ui/file_manager/integration_tests/file_manager/quick_view.js

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 13 2018

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

commit 2111e0cf2eec22a80d0492eeb121686d281c98e5
Author: Noel Gordon <noel@chromium.org>
Date: Wed Jun 13 08:47:35 2018

Increase QuickView browser test coverage

With more speed, add more coverage: run open Quick View case in Tablet
mode, and in Guest mode (so easy). Add a Quick View open test case for
an ephemeral volume (USB here).

Tablet (aka fully immersive) mode is not supported in Mash: add a Mash
bot exclusion for the Tablet mode case.

Bug:  851888 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I893f35838731b3264268b027a00a86c339bf5b3f
Reviewed-on: https://chromium-review.googlesource.com/1098432
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566761}
[modify] https://crrev.com/2111e0cf2eec22a80d0492eeb121686d281c98e5/chrome/browser/chromeos/file_manager/file_manager_browsertest.cc
[modify] https://crrev.com/2111e0cf2eec22a80d0492eeb121686d281c98e5/testing/buildbot/filters/mash.browser_tests.filter
[modify] https://crrev.com/2111e0cf2eec22a80d0492eeb121686d281c98e5/ui/file_manager/integration_tests/file_manager/quick_view.js

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 14 2018

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

commit 58ed5a3222012df0952d1f5e67671eb45ea861fd
Author: Noel Gordon <noel@chromium.org>
Date: Thu Jun 14 01:12:02 2018

Use descendant combinator in openQuickViewUsb browser test

Code search for USB_VOLUME_QUERY suggests many uses of child selectors
from the #directory-tree. Alternative is to use descendant combinator:
#directory-tree [volume-type-icon="xxxxx"] to query for volumes, which
is shorter and more idiomatic with the volume code selectors.

Unless we need to test a child element for [active], or similar, it is
perhaps less confusing to use the descendant combinator [1].

[1] Future changes might clean up the use of child selectors in volume
MTP|USB|ETC_VOLUME_QUERY in the relevant test cases.

No change in behavior, no new tests.

Test: browser_test --gtest_filter="QuickView/FilesApp*"
Bug:  851888 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I2f64c624e05d0051750ee49ed6e7fabbef43b512
Reviewed-on: https://chromium-review.googlesource.com/1098905
Reviewed-by: Naoki Fukino <fukino@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567075}
[modify] https://crrev.com/58ed5a3222012df0952d1f5e67671eb45ea861fd/ui/file_manager/integration_tests/file_manager/quick_view.js

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 14 2018

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

commit 2b6aa94a27ca8ae0d38857b9bf8212bdebf7a2c1
Author: Noel Gordon <noel@chromium.org>
Date: Thu Jun 14 01:13:56 2018

Use descendant combinator in copyBetweenWindows browser tests

Use a decendant combinator selector to query for the USB volume: it is
shorter and more idiomatic with other volume selector JS test code.

Test: browser_test --gtest_filter="*FilesApp*copyBetweenWindows*"
Bug: 836254, 851888 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I70b9ecad9e57db6a2d0028ba329707c5574710e8
Reviewed-on: https://chromium-review.googlesource.com/1098908
Reviewed-by: Naoki Fukino <fukino@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567077}
[modify] https://crrev.com/2b6aa94a27ca8ae0d38857b9bf8212bdebf7a2c1/ui/file_manager/integration_tests/file_manager/copy_between_windows.js

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 14 2018

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

commit ab508cbb92101f3db0144c0ba727afa35b551330
Author: Noel Gordon <noel@chromium.org>
Date: Thu Jun 14 01:14:10 2018

Use descendant combinator in FileDisplay USB,MTP browser tests

Use a descendant combinator CSS selector to query for USB/MTP volumes:
it is shorter and more idiomatic with volume code selectors.

Other clean-up while here: add a comment for load File app step. Where
an argument in a test step function is unused, remove it.

Test: browser_test --gtest_filter="FileDisplay/FilesApp*"
Bug: 836254, 851888 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I88b04907ed04b355d8de9412ba748ca1ce0ced21
Reviewed-on: https://chromium-review.googlesource.com/1098907
Reviewed-by: Naoki Fukino <fukino@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567078}
[modify] https://crrev.com/ab508cbb92101f3db0144c0ba727afa35b551330/ui/file_manager/integration_tests/file_manager/file_display.js

Comment 9 by noel@chromium.org, Jun 19 2018

Cc: sa...@chromium.org lucmult@chromium.org
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 19 2018

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

commit ab55380088b936ecffc895080227a36e3d6f5849
Author: Noel Gordon <noel@chromium.org>
Date: Tue Jun 19 09:25:35 2018

Simplify setupAndWaitUntilReady in QuickView browser tests

The QuickView tests are slower than most, and the bug addresses making
them faster. setupAndWaitUntilReady loads, and pre-populates Downloads
and Drive volumes with many file entries. Can we do better?

A modest test speed improvement is to make setupAndWaitUntilReady only
populate the volumes needed in the tests (Downloads, Drive, etc), with
only those files needed to perform each test.

Bug:  851888 
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: I0cb4d08fa38d9df76999595f41dfb8a0df378a9f
Reviewed-on: https://chromium-review.googlesource.com/1104619
Reviewed-by: Sam McNally <sammc@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568378}
[modify] https://crrev.com/ab55380088b936ecffc895080227a36e3d6f5849/ui/file_manager/integration_tests/file_manager/quick_view.js

Project Member

Comment 11 by bugdroid1@chromium.org, Jun 19 2018

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

commit 8615321de3032322501be23dde2f71740342bb50
Author: Noel Gordon <noel@chromium.org>
Date: Tue Jun 19 13:02:19 2018

Document the Quick View browser test helpers

Now the Quick View browser test suite has been optimized for speed, we
can finish off by documenting the Quick View test JS. Two test helpers
only lack documentation: document what they do.

Comment only change: no change in behavior, no new tests.

Bug:  851888 
Change-Id: If87c49919cdd3bd51fa35f714130fc74fb84b543
Reviewed-on: https://chromium-review.googlesource.com/1105530
Reviewed-by: Sam McNally <sammc@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568419}
[modify] https://crrev.com/8615321de3032322501be23dde2f71740342bb50/ui/file_manager/integration_tests/file_manager/quick_view.js

Comment 12 by noel@chromium.org, Jun 19 2018

Labels: OS-Chrome
At this point, the simplest possible file is used to open QuickView (#3 use a text file rather than a PNG), and minimize the number of files added to the test volumes during test (#10).

This makes the current Quick View tests about as fast as possible. They are not doing anything silly, like playing 5 seconds of Audio on the bots. They're very basic tests of a Files.App feature (Quick View), and coverage has been expanded in this bug.

The litmus test then is, what are the affects on MSAN/ASAN bots with all these changes?  Have things improved?  In a few days, we'll have that data...

Comment 13 by noel@chromium.org, Jun 29 2018

Cc: kbr@chromium.org dpranke@chromium.org
Status: Fixed (was: Started)
The recent speed-up in the MSAN bots was noted in related FilesApp browser test bugs.  I'm yet to identify the change that improved it.

But the first effect was test-time reduced by about 30%. Instead of the FilesApp QuickView tests persistently hitting the MSAN bot time-out (180s) with a TIMEOUT PASS, TIMEOUT PASS ... flake, they improved to have average test-time 138s, with no flakes at all.

More work must have happened on the MSAN bot configs as of today ... the last 4 runs of the FilesApp QuickView tests on MSAN now average 40s.

We have gone from 180s test-run-time and TIMEOUT PASS flakes, to 40s test-run-time and no flakes as of today [1].

https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=QuickView%2FFilesAppBrowserTest

^^^ result looks very good to me: green everywhere RELEASE/DEBUG/MSAN/ASAN and no flakes.

Comment 14 by kbr@chromium.org, Jun 29 2018

Nice work Noel.

Comment 15 by noel@chromium.org, Jun 30 2018

Thanks Ken.  I was wondering if you or Dirk knew of any recent infra work on the chrome-os MSAN bots?

Those bots became way faster in the last week or so.  Someone else in the team has found a massive MSAN perf win by fixing a bug or reverting something.

I noticed all the FilesApp browser test suite (that's 270 fixtures) became faster on MSAN this week.  The tests in this bug (180/40 - 1) * 100% = 350% faster.

Comment 16 by kbr@chromium.org, Jun 30 2018

Sorry, I don't know anything about those bots.

No worries. Snapshot of the flakes going away and the speed increases.
QuickView:FilesAppBrowserTest.Test:closeQuickView.png
140 KB View Download
Status: Verified (was: Fixed)

Sign in to add a comment