De-duping. The test should handle this particular failure without making the bots red in subsequent runs.

Like a teardown function after each test which kill any running chrome/installer process, then delete all existed directories?

And I still not get the reason of first failure here:
[0306/175835.349:ERROR:uninstall.cc(132)] Failed to delete path: C:\Users\chrome-bot\AppData\Local\Chromium\Application\59.0.3033.0\Installer\setup.exe
Is it because the installer doesn't quit normally and uninstaller can't delete file then?

It's because MoveFile doesn't work across volumes: https://cs.chromium.org/chromium/src/chrome/installer/setup/uninstall.cc?sq=package:chromium&type=cs&q=todo%5C(tommi%5C)+file:uninstall%5C.cc&l=1072

The test harness already has partial code to clean up after a test failure (RunCleanCommand with force_clean=true). This cleans up various registry state (DeleteGoogleUpdateRegistration). It should, in addition, clean up everything else that is verified by the "clean" state. Maybe this could be done in a data-driven way: walk the "clean" state with new code that clears the registry and filesystem of everything needed to make the "clean" state valid.

[quote]It's because MoveFile doesn't work across volumes[/quote]
So sometimes tmp dir is located in different volumes on trybots? Sounds like a failure could also happened for normal user? Can uninstaller create a temp dir on the same volumes in that situation?

[quote]Maybe this could be done in a data-driven way[/quote]
Agree, and kill the process. Actually, I think the cleanup should verify and kill the process first because it may block filesystem cleanup. 

So what if the cleanup failed. For example, a mystery process we don't know hold the file that needs to be deleted. Is it worth to handle that situation?

About process, there is config to verify the chrome is not running. However, it's not true for installer/uninstaller process, is there any reason we don't do that?

This bug was opened as a result of r699022, which was caused by a change in the test infrastructure that put TMP/TEMP on a different volume. Yes, this can happen for normal users. In this case, I believe that setup.exe does a reasonable effort to clean up to the extent that it can. It's possible that we could do more. The short-term priority is making the test work properly when uninstall fails for any reason at all.

Chrome is not currently started by the test, so there is no need today to kill it. In time, I would like to re-introduce launching Chrome (https://codereview.chromium.org/1804983002/). There are already provisions in quit_chrome.py to terminate all procs if they don't exit cleanly.

If the test harness can't clean up, then we need diagnostics in the test harness to help us understand why. At the moment, however, there is no evidence that this is needed.

r699022 -> issue 699022

I can make RunCleanCommand based on the "clean" state.

Also, I don't think that will fix the flaky tests issue that is caused by uninstaller failure. It just makes sure one failed test won't affect the following ones.

I guess the uninstaller failure will be fixed by  issue 699327 ?

Thanks for volunteering for this, Owen! I think that the primary goal for this issue is to fix the test harness so that a failure in test run R1 doesn't lead to failures in test run R2+. Making RunCleanCommand(force_clean=True) satisfy the requirements of the "clean" state should be enough to make the harness gracefully handle any sort of failure from any stage of any test (a huge win). If RunCleanCommand can't sufficiently clean the machine, then I think the right thing is for there to be blinking red test failures so we know that we need to investigate specific failures in more detail.

The specifics of TEMP/TMP should be tracked in another issue. The questions in my  mind are:
- is the installer doing the best it can possibly do when TEMP is on another volume?
- is the installer reporting its status properly given the above? maybe "i had no choice but to leave some files behind" shouldn't be reported as a failure.
- does the test harness need to know the difference between "uninstalled and all files removed" vs "uninstalled but some files were left behind"?
- should a test pass if certain files weren't removed during uninstall (those files should be removed by the test harness before running the next test)?

As a user, I think any software uninstaller should delete everything related. If it left something behind, I'll think "ah-ha there's a bug here". No, not all softwares on desktop/cellphone cleanup everything after uninstall. Personally, I don't think that is a very good UX.

That's why for the test, it's ok to say this is not our priority issue. So before it's fixed, we suppress the check to avoid flaky failure. But I don't want to define the "uninstalled but some files were left behind" as a success uninstall.

Also, yes, if we can get the metrics about uninstall failure, I think it's a good idea to report a different status if uninstaller can't remove all files on disk.

I guess the real challenge here is self-deleting? Maybe we can copy the setup.exe to the TMP at the beginning of uninstall and use the new setup.exe to uninstall Chrome? 
I found a post here about self-deleting:
http://www.catch22.net/tuts/self-deleting-executables

 

Yes, I agree that it's a bug that uninstall may leave files behind. I have created issue 700809 for setup.exe itself. Let's keep this issue focused on making the test robust, which is an independent task. The test should assert that all files are removed on successful uninstall, and that all uninstalls are successful. I believe this is what it's currently doing. The gap is that it doesn't clean up after a failed uninstall so that subsequent tests may pass.

As for the "delete the current exe" tricks, we are currently using one to delete setup.exe after moving it to TMP; see DeleteFileFromTempProcess. As I mentioned in issue 700809, perhaps we should handle the failure to move to TMP by moving elsewhere on the same volume and then using the temp process trick on those files. Please continue the discussion of this in the other issue.