Thank you for finding this.
Apparently, the said tests are not doing what they are intended to do.
Probably we should get rid of MockHomedirs after fixing the said tests.

> Probably we should get rid of MockHomedirs after fixing the said tests.

BTW, this is exactly how I discovered this issue. I was trying this for bug 832395 (CL https://crrev.com/c/1098255), as a way to avoid changing half of the mocking code in the 3000-line file mount_unittests.cc.


Although, in general, one may argue that for *unit* tests of class Foo it makes sense to use only *mocked* versions of classes on which Foo depends. Otherwise the test becomes more like an integration test, and we get into the nightmare of mocking out tons of low-level operations (which is exactly what has accumulated in mount_unittests.cc since years and make_tests.cc - see hundreds of mock expectations for FileExists(), ReadFile(), etc.).
But that's, probably, a conceptual question which is detached from reality where no one would like to rewrite 3K lines of test code :)

Files that are deleted in HomeDirs::Remove() are:
/home/.shadow/<user_hash>
/home/user/<user_hash>
/home/root/<user_hash>

Shadow root is not user in ephemeral case.
The last two are only mount points for created cryptohome, so removing them is only user presence clean up.

These directories are handled outside of MountEphemeralCryptohome(), so it's correct that they are removed when MountEphemeralCryptohome fails, however, test description is incorrect and some cleanup happens.

But, IIUC, "/home/.shadow/<user_hash>" normally contains the user data - is it intended that it's cleared there? It's possible in theory that there's some actual data left there from when the user had been not ephemeral; although we probably don't care about this old data.

In any case, looks like the main confusion arises from the unclear meaning of the "cleanup" word. Or, to put it differently, which kind of cleanup was considered as unwanted for this feature?

It should be OK to delete "/home/.shadow/<user_hash>" as this directory should be deleted as soon as user becomes ephemeral.

I agree that the main confusion is "cleanup" meaning here. Actually, the test is even not the best one as it just confirms very special behavior and even if the cleanup is happening, it doesn't brake anything as the data/directories should not exist anyway. It just checks that no unnecessary extra work is done.

Do you think it will be OK to just update the wording here + check that Homedirs::Remove() is called anyway as it's OK behavior? Or should the tests be updated to not use MockHomedirs anymore?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/b39ae7f9c6e18e6daf96d17baec810fe956c0670

commit b39ae7f9c6e18e6daf96d17baec810fe956c0670
Author: Maksim Ivanov <emaxx@chromium.org>
Date: Tue Jul 31 23:37:23 2018

cryptohome: Use HomeDirs for loading VK in Mount

Remove the duplicated logic from Mount, and use the already
existing functionality provided by HomeDirs instead.

Also fix comments in unit tests for ephemeral mount, in which
some cleanup was actually happening despite the comments were
claiming the opposite.

BUG=chromium:832395, chromium:853012 
TEST=existing tests

Change-Id: I91845739f1bc80f2f1bcd276159221a8256a9f87
Reviewed-on: https://chromium-review.googlesource.com/1098255
Commit-Ready: Maksim Ivanov <emaxx@chromium.org>
Tested-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>

[modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/mock_homedirs.h
[modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/homedirs.h
[modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/vault_keyset_unittest.cc
[modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/vault_keyset.h
[modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/mount.cc
[modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/crypto_unittest.cc
[modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/vault_keyset.cc
[modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/mock_vault_keyset.h
[modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/crypto.h
[modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/crypto.cc
[modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/homedirs_unittest.cc
[modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/homedirs.cc
[modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/mount_unittest.cc

Is it fixed?

Yes.