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

commit 6e10377454c93bca2b0423270ebdb5cd6b4c3ae5
Author: Joel Hockey <joelhockey@chromium.org>
Date: Tue May 22 02:23:35 2018

FilesApp crostini UI test

Added a test for FilesApp crostini integration using UI frontend tests.

* Reenabled FileManagerUITest to run this test and others.
* Removed code where crostini dir is selected in background thread
  when mount completes.  This code is not required since crostini
  is never mounted without an existing FilesApp window.  There is
  similar code in DirectoryModel which takes care of this.
* Modifed DirectoryModel onVolumeInfoListUpdated_ to change directory
  to crostini even if window is not focussed.  This is helful in
  tests and still relevant in non-test mode where opening crostini
  may take 5-10s and users may switch windows.  Crostini dir should
  still be selected if window is not focussed.
  Window focus check is still done for PROVIDED filesystems.
* Refactored FileManager.setupCrostsini_ to allow for better testing.

Bug:  845075 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I13ce875cb23cc435a95cefc19cac892d9aed4eec
Reviewed-on: https://chromium-review.googlesource.com/1065556
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560461}
[modify] https://crrev.com/6e10377454c93bca2b0423270ebdb5cd6b4c3ae5/chrome/browser/chromeos/file_manager/file_manager_uitest.cc
[modify] https://crrev.com/6e10377454c93bca2b0423270ebdb5cd6b4c3ae5/ui/file_manager/file_manager/background/js/background.js
[modify] https://crrev.com/6e10377454c93bca2b0423270ebdb5cd6b4c3ae5/ui/file_manager/file_manager/foreground/js/directory_model.js
[modify] https://crrev.com/6e10377454c93bca2b0423270ebdb5cd6b4c3ae5/ui/file_manager/file_manager/foreground/js/file_manager.js
[add] https://crrev.com/6e10377454c93bca2b0423270ebdb5cd6b4c3ae5/ui/file_manager/file_manager/test/crostini.js
[modify] https://crrev.com/6e10377454c93bca2b0423270ebdb5cd6b4c3ae5/ui/file_manager/file_manager/test/js/chrome_file_manager.js
[modify] https://crrev.com/6e10377454c93bca2b0423270ebdb5cd6b4c3ae5/ui/file_manager/file_manager/test/js/strings.js
[modify] https://crrev.com/6e10377454c93bca2b0423270ebdb5cd6b4c3ae5/ui/file_manager/file_manager/test/js/test_util.js

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

commit 3ef628a1722529f71e0f532e90c13b84842ac368
Author: Joel Hockey <joelhockey@chromium.org>
Date: Tue May 22 06:10:18 2018

FilesApp fix crostini test to wait for callback

Explicitly wait for callback to executed.  Don't assume that it has
been executed when progress is displayed.

Bug:  845075 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Ib6ad4186894e4785dfdb1d08f01a11625a055f54
Reviewed-on: https://chromium-review.googlesource.com/1068629
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560495}
[modify] https://crrev.com/3ef628a1722529f71e0f532e90c13b84842ac368/ui/file_manager/file_manager/test/crostini.js

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

commit 1947c722c19cf47871095848a84e8134637807dc
Author: Joel Hockey <joelhockey@chromium.org>
Date: Wed May 23 10:26:25 2018

FilesApp crostini test show downloads when crostini gone

Bug:  845075 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I58042d110a399d6c4d6879132292f9d75a99ad9a
Reviewed-on: https://chromium-review.googlesource.com/1070087
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561025}
[modify] https://crrev.com/1947c722c19cf47871095848a84e8134637807dc/ui/file_manager/file_manager/test/crostini.js

Please don't add tests to /test/.

Could we add some:
- Browser tests, to ensure we test the FMP API part of crostini, with a mocked out file system
- Unit tests, for the navigation bar (maybe these already exist?)

Marking this as blocking crbug.com/822130, since its part of increasing coverage of the FMP API.

I'm not sure how /test got enabled again, or why we are making changes to it now.

I thought the discussion around using /test was resolved as "we are not going to commit to developing and maintaining yet another testing system", and we focused on moving all our tests to integration_tests/ ( issue 813477  and  issue 825659 ). We've been investing in improving these - Noel has done a great job making them reliable, as well as working to get them to run in tablet/clamshell mode, Stuart and I are getting Team Drives supported, etc - so I don't see a good reason to support another testing system.

As a team, I think we need to agree on a testing framework we are all committed to, so we can focus on documenting it and making it work well. If there are gaps in functionality that you need for testing Crostini, we should focus on adding support for that.

Also, I think integration tests are a great way to test Crostini - you can create a mock Crostini backend (like what we have for Drive) which can fail in weird ways etc, and test that the frontend responds accordingly. Using /test will only test the front-end, missing issues caused from the FMP API.

I understand if this is an intermediate step, with a longer-term plan to get these converted into integration tests, but that needs to happen before M69. Keen to hear your thoughts, Joel :)

I think that we should use the UI tests (FileManagerUITest).  I wouldn't describe the UI tests as another testing system.  They are basically a special case of our unit tests (FileManagerJsTest) which load the full html and JS.  They run in the same InProcessBrowserTest, and use the same webui_resource_test runner.  The FileManagerUITest class is just like thousands of other InProcessBrowserTest classes we have and sheriffs would have no problem to disable, etc if ever needed.

I'm not sure about your quote.  Was it in one of the issues?  I turned the UI tests off in prod while Noel was taking a larger overall look at tests with the understanding that we would reevaluate when he had a better handle on the integration tests.  Earlier this week we reenabled the UI tests since they are a good way to test the code I have done for crostini.  

For crostini, the main focus right now for tests is to use the autotests in chromeos.  I believe that smbarber is looking into this, and it is also on Ben's radar.  I plan to get involved in this to make sure we are testing the FileManager integration.

The crostini code in FMP is minimal.
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/extensions/file_manager/private_api_misc.cc?type=cs&q=FileManagerPrivateMountCrostiniContainerFunction::RunAsync&g=0&l=667
If we were to write tests to execute this code, I don't see that it would achieve much.  We would need to mock CrostiniManager, and DiskMounter, and then use a fake volume in VolumeManager.  Adding autotests will give us code coverage for this area.

We have been told it's not possible to write autotests for UI code, so it's doubt this is an avenue to rely on for test coverage.

I would like to see browser tests for crostini integration, mocking/faking out the backends is what we already do for drive and does not seem to be overly burdensome, and then we can leverage the work Noel has done to run tests in clamshell/tablet mode.

+nverne who is looking at crostini testing.  We will have to rely on something like autotests (or whatever Nick ends up using for his code) to get test coverage for the FilesApp crostini integration.  It is the only way we will be able to catch regressions where code / configuration in concierge / cicerone / garcon / container changes and breaks our integration.  We wont be able to do it with our FileManagerBrowserTests.

Drive and Crostini are superficially similar in that they are both network filesystems, but within our VolumeManager, they are very different (but will become a little more similar when we use drivefs).  It is not a helpful comparison for the implementation or testing to compare them.

In both prod and test code, Drive registers a mount with VolumeManager during initialization.
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/file_manager/volume_manager.cc?l=426&rcl=a45ed35b300d0900e5d38c145d8625834c12eefd
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc?l=612&rcl=a45ed35b300d0900e5d38c145d8625834c12eefd

For Crostini, once the mount is created, it doesn't exercise any different codepaths to Downloads.  Downloads points at /home/chronos/user/Downloads, crostini points at /media/fuse/crostini_<id>_termina_penguin.
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/file_manager/volume_manager.cc?l=329&rcl=095fe4f2eb66f97cb3a4029b3a7cf7aa7d492a44

The part that we need to test for crostini to make sure we don't get regressions, is the code that triggers the mount and then interacts with the other systems to create the mount.  I've got the UI tests already implemented, so even if autotests aren't able to interact with UI much, they still seem to me to be the right approach.

Thanks Joel. I have some concerns about the UI testing system, I'm not going to discuss them all here. I think as a team we need to come together and agree on the way forward with testing. I'd also like to hear Noel's opinion as he has been working on improving tests for File Manager.

I've booked some time on Wednesday morning to discuss our testing plan for the Files App.

When we add new File Manager Private API, it needs a test: see all the tests in chrome/test/data/extensions/api_test/file_browser.  For Drive, drive_search_test is a good example, and it was where FakeDriveService C++ was first introduced and used for testing (we don't run Drive in chrome prod).  Crib from that for Crostini, add a C++ FakeCrostiniService.

FakeDriveService was later used in the Files.App browser tests [1], to add tests of Drive integration with Files.App and prevent regressions (refer to the change description details). A C++ FakeCrostiniService would do similarly, add tests.

Browser tests / autotests are separate systems, so browser tests won't and can't help with whatever autotest-ing is planned, that is a separate question.  Having autotests does not remove the need for browser tests.

[1] https://codereview.chromium.org/search?closed=1&owner=dhaddock%40chromium.org&reviewer=&cc=&repo_guid=&base=&project=&private=1&commit=1&created_before=&created_after=&modified_before=&modified_after=&order=&format=html&keys_only=False&with_messages=False&cursor=&limit=30

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

commit b4268a4de0dfd26787354ecab6e1298d1c04cf15
Author: Joel Hockey <joelhockey@chromium.org>
Date: Thu May 31 05:13:59 2018

Add a fileManagerPrivate.isCrostiniEnabled test

Additional test for fileManagerPrivate.mountCrostiniContainer
will be added to these files next.

Bug:  845075 , 822130
Change-Id: I8e4fbb840764e975b5d670556905f3b4b411f2da
Reviewed-on: https://chromium-review.googlesource.com/1075878
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563153}
[modify] https://crrev.com/b4268a4de0dfd26787354ecab6e1298d1c04cf15/chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc
[add] https://crrev.com/b4268a4de0dfd26787354ecab6e1298d1c04cf15/chrome/test/data/extensions/api_test/file_browser/crostini_test/manifest.json
[add] https://crrev.com/b4268a4de0dfd26787354ecab6e1298d1c04cf15/chrome/test/data/extensions/api_test/file_browser/crostini_test/test.js

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

commit 817a0f93053534947536a8c98811aa541f5e9926
Author: Joel Hockey <joelhockey@chromium.org>
Date: Fri Jun 01 01:35:50 2018

fileManagerPrivate.mountCrostiniContainer test

* Added CrostiniManager::SkipRestartForTesting
  to bypass component loading and starting
  concierge to start VM and container.
* Added MockDiskMountManager::NotifyMountEvent
  to trigger OnMountEvent to observers.

Bug:  845075 
Bug: 822130
Change-Id: Ifd9464730c6cd50a683cc40f60a289c80a36158c
Reviewed-on: https://chromium-review.googlesource.com/1080487
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Nicholas Verne <nverne@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563476}
[modify] https://crrev.com/817a0f93053534947536a8c98811aa541f5e9926/chrome/browser/chromeos/crostini/crostini_manager.cc
[modify] https://crrev.com/817a0f93053534947536a8c98811aa541f5e9926/chrome/browser/chromeos/crostini/crostini_manager.h
[modify] https://crrev.com/817a0f93053534947536a8c98811aa541f5e9926/chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc
[modify] https://crrev.com/817a0f93053534947536a8c98811aa541f5e9926/chrome/test/data/extensions/api_test/file_browser/crostini_test/manifest.json
[modify] https://crrev.com/817a0f93053534947536a8c98811aa541f5e9926/chrome/test/data/extensions/api_test/file_browser/crostini_test/test.js
[modify] https://crrev.com/817a0f93053534947536a8c98811aa541f5e9926/chromeos/disks/mock_disk_mount_manager.cc
[modify] https://crrev.com/817a0f93053534947536a8c98811aa541f5e9926/chromeos/disks/mock_disk_mount_manager.h

UI tests and FMP unittests added.
end-to-end testing to be addressed in  crbug.com/849438 

This is not finished.

The linked bug is for tests on the Crostini side; we still need tests on the Files App side, which includes double-clicking the Crostini icon, checking it can mount correctly and files appear, checking we can drag/drop files to/from the Crostini container and they transfer correctly, checking we can write files to the container, checking the navbar updates when we navigate inside the container, checking the container is unmounted when we eject it, etc. We can do this with integration tests. 

Is there an integration test plan for the Crostini feature? If not, we should make one and aim for 100% coverage. Please speak to noel@ about adding integration tests for Crostini.

The linked bug is to use autotests or tast to do an integration test of crostini.

I will either use the same bug for the FilesApp testing that is part of it, or I 
will create a new bug.

I am closing this bug since it is being used to track progress for crostini tasks and the work is now done that it was tracking.

The testing that will be done is to use FilesApp to start/mount container, verify container shutdown behaviour, and copying files between volumes.

I would gladly test these things in FileManagerBrowserTest if I could since it is a simpler testing env than autotests.  But it is not possible to test these things and ensure that we don't get the sort of regressions we have been seeing in crostini by using FileManagerBrowserTest and mocking everything out.

I think that I currently have close to 100% code coverage for the FilesApp crostini code.  Are there specific parts of the code that you think aren't currently covered?

I would like to audit the tests we have and ensure what we have and what we plan to have makes sense. 

Will report back on this bug once the audit is completed with results and next steps, if any.

I will add a new integration_test to mount crostini by clicking on Linux Files icon and verifying files are displayed.  My reluctance so far is that I don't believe it adds any coverage or will catch any regressions.

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

commit bafb0d979c467eed0b1123eed13fe683b795279c
Author: Joel Hockey <joelhockey@chromium.org>
Date: Thu Jun 14 08:41:32 2018

FilesApp mountCrostiniContainer unittest verify DiskMountManager params

Bug:  845075 
Change-Id: Id6667e02fcdd7be54d11e6ccec0074a475948754
Reviewed-on: https://chromium-review.googlesource.com/1100360
Reviewed-by: Stuart Langley <slangley@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567193}
[modify] https://crrev.com/bafb0d979c467eed0b1123eed13fe683b795279c/chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc

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

commit 6670570c8283f3cd02e4e99fe72c7e2f87c2e217
Author: Joel Hockey <joelhockey@chromium.org>
Date: Fri Jun 29 00:37:49 2018

FilesApp crostini integration test

* Test verifies fake root is shown, then mounts container
  and verifies that correct volume and contents are displayed.
  Unmounts volume, and verifies that fake root is shown again.
* Sets prefs / features for crostini and registers mount callback
  with FakeCrosDisksClient to simulate sshfs mounting.

Bug:  845075 
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: Ice3f96430ef063a9942fdd2c40e99b30b35b0c50
Reviewed-on: https://chromium-review.googlesource.com/1094574
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Stuart Langley <slangley@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571344}
[modify] https://crrev.com/6670570c8283f3cd02e4e99fe72c7e2f87c2e217/chrome/browser/chromeos/file_manager/file_manager_browsertest.cc
[modify] https://crrev.com/6670570c8283f3cd02e4e99fe72c7e2f87c2e217/chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc
[modify] https://crrev.com/6670570c8283f3cd02e4e99fe72c7e2f87c2e217/chrome/browser/chromeos/file_manager/file_manager_browsertest_base.h
[modify] https://crrev.com/6670570c8283f3cd02e4e99fe72c7e2f87c2e217/chrome/browser/chromeos/file_manager/volume_manager.cc
[modify] https://crrev.com/6670570c8283f3cd02e4e99fe72c7e2f87c2e217/chrome/browser/chromeos/file_manager/volume_manager.h
[modify] https://crrev.com/6670570c8283f3cd02e4e99fe72c7e2f87c2e217/chromeos/components/drivefs/fake_drivefs.cc
[modify] https://crrev.com/6670570c8283f3cd02e4e99fe72c7e2f87c2e217/chromeos/components/drivefs/fake_drivefs_launcher_client.cc
[modify] https://crrev.com/6670570c8283f3cd02e4e99fe72c7e2f87c2e217/chromeos/dbus/fake_cros_disks_client.cc
[modify] https://crrev.com/6670570c8283f3cd02e4e99fe72c7e2f87c2e217/chromeos/dbus/fake_cros_disks_client.h
[modify] https://crrev.com/6670570c8283f3cd02e4e99fe72c7e2f87c2e217/ui/file_manager/integration_tests/file_manager/background.js
[add] https://crrev.com/6670570c8283f3cd02e4e99fe72c7e2f87c2e217/ui/file_manager/integration_tests/file_manager/crostini.js
[modify] https://crrev.com/6670570c8283f3cd02e4e99fe72c7e2f87c2e217/ui/file_manager/integration_tests/file_manager/folder_shortcuts.js
[modify] https://crrev.com/6670570c8283f3cd02e4e99fe72c7e2f87c2e217/ui/file_manager/integration_tests/file_manager_test_manifest.json