Add unit test coverage for racing updaters

[hlwarriner]

It would be good to also add some coverage in
update_client::UrlFetcherDownloader; we can plan to do that in a separate change.

b/309841211

Change-Id: I6505843004aafc920a1a7e84a6c61551dbbbb7a3

[hlwarriner]

@oxve @sideb0ard, please let me know if you have any feedback on the style of the tests. For example, an argument could be made to stub or fake out the drain file / file system dependency in CobaltSlotManagementTest.ConfirmSlotBailsIfOtherAppStartedDrainFirst. I chose to use the real file system for a few reasons, though: to keep the test more realistic, especially since we aren't able to have an end-to-end test for this scenario; other tests in this suite are already using the real file system, and it seems like it hasn't been too slow; it wasn't too difficult here to set up the files that are required.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (79920bc) 58.53% compared to head (46fe5e3) 58.55%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2292      +/-   ##
==========================================
+ Coverage   58.53%   58.55%   +0.02%     
==========================================
  Files        1904     1904              
  Lines       94522    94522              
==========================================
+ Hits        55324    55346      +22     
+ Misses      39198    39176      -22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[oxve]

@oxve @sideb0ard, please let me know if you have any feedback on the style of the tests. For example, an argument could be made to stub or fake out the drain file / file system dependency in CobaltSlotManagementTest.ConfirmSlotBailsIfOtherAppStartedDrainFirst. I chose to use the real file system for a few reasons, though: to keep the test more realistic, especially since we aren't able to have an end-to-end test for this scenario; other tests in this suite are already using the real file system, and it seems like it hasn't been too slow; it wasn't too difficult here to set up the files that are required.

I don't see an issue with using real files. Like you say, it follows the existing pattern.

What files is this change supposed to increase coverage in? Looking at the codecov report I couldn't see any obvious candidates. I want to make sure that there aren't more files that are missing in the coverage report.

[hlwarriner]

@oxve @sideb0ard, please let me know if you have any feedback on the style of the tests. For example, an argument could be made to stub or fake out the drain file / file system dependency in CobaltSlotManagementTest.ConfirmSlotBailsIfOtherAppStartedDrainFirst. I chose to use the real file system for a few reasons, though: to keep the test more realistic, especially since we aren't able to have an end-to-end test for this scenario; other tests in this suite are already using the real file system, and it seems like it hasn't been too slow; it wasn't too difficult here to set up the files that are required.

I don't see an issue with using real files. Like you say, it follows the existing pattern.

What files is this change supposed to increase coverage in? Looking at the codecov report I couldn't see any obvious candidates. I want to make sure that there aren't more files that are missing in the coverage report.

Cool, sounds good.

Re coverage, this change adds tests that cover important behaviors in two modules, but the impact on actual covered lines should be very small (or possibly zero)...

Coverage should be added for lines 191 and 192 at https://github.com/youtube/cobalt/blob/main/components/update_client/cobalt_slot_management.cc#L191. But I think this entire file is excluded from coverage reports since it isn't under //cobalt/ or //starboard/, right?

No new lines are covered in https://github.com/youtube/cobalt/blob/main/starboard/loader_app/drain_file.cc; the new test should just cause false to be returned from RankAndCheck() for a new reason on line 187.

[hlwarriner]

@y4vor can you please take a look at the new tests?