Thanks for tracking this down!

It looks like we don't have many tests using CGEventPost - AppShimMenuControllerUITest has

  // Send Cmd+`. Note that it needs to go into kCGSessionEventTap, so NSEvents
  // and [NSApp sendEvent:] doesn't work.
  void CycleWindows() {
    bool key_down = true;  // Sending a keyUp doesn't seem to be necessary.
    base::ScopedCFTypeRef<CGEventRef> event(
        CGEventCreateKeyboardEvent(nullptr, kVK_ANSI_Grave, key_down));
    EXPECT_TRUE(event);
    CGEventSetFlags(event, kCGEventFlagMaskCommand);
    CGEventPost(kCGSessionEventTap, event);
  }


Likely that "Sending a keyUp doesn't seem to be necessary." is totally wrong! We should just CGEventPost a keyUp as well so that this test isn't leaving global state on the bots. That's the approach we usually take, rather than taking steps to clear global state as a precaution.

I do still like the idea of *checking* in interactive_ui_tests_main.cc, but it should cause the test leaving global state to fail, rather than attempt to clean it up.

The only other test using CGEventPost is chrome/browser/extensions/extension_commands_global_registry_apitest.cc which seems to send keyups, so that's all good. If AppShimMenuControllerUITest is the only test causing this issue, then we should fix the test.

It's also often good to remove reliance on global state from tests. That is, by avoid any calls to [NSEvent modifierFlags] being invoked in any codepath that would occur in a test and cause a flake. This can be done with a mock, swizzling out modifierFlags (e.g. like in ui/views/test/event_generator_delegate_mac.mm for pressedMouseButtons), or by tweaking code to use properties on the NSEvent instance rather than the class/global object.

There's one more use-case for cleaning up: what if a test crashes, or hangs, or one of it's ASSERTs fail (because it's flaky) and it happens after key down but before key up? (All of those are highly unlikely with extension_commands_global_registry_apitest.cc, but still.) If we don't clean it up, the bot will be left in a dirty state. I just feel that tests should be run in an isolated and controlled environment: you clean it up to the known state before it runs, and you make sure to return it to the original state afterwards. As for avoiding global state - yes, you should avoid it whenever possible, but sometimes you want to write integration tests that do work with the global state, so that the test doesn't just look like an end-user scenario, it is an end-user scenario.

Your points are valid, but there's a lot of other global state that we don't attempt to return to a consistent state.

Things like clipboard, filesystem, CapsLock state, and a swathe of system settings (e.g. Ctrl+F7 "Full Keyboard Access" on Mac, "Show scroll bars" behaviour, ..).

The current way we approach this is to ensure individual tests are well behaved when they don't crash, and to use mocks/swizzlers (e.g. scoped_preferred_scroller_style_mac.h and test_clipboard in ui/base/test). We also simply reboot a lot of trybots between runs.

So a step just to clear shift/cmd/ctrl modifiers feels like we're making an exception just to cater for one badly behaved test.

Okay, I've updated my CL accordingly.

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

commit 368836b81249724242221c47eff1106485b54da6
Author: Alexander Shabalin <alshabalin@yandex-team.ru>
Date: Wed Mar 07 06:56:45 2018

Mac: Fail tests that leave modifier keys hanging.

Adds ui_test_utils::SendGlobalKeyEventsAndWait and updates two
interactive_ui_tests that could leave hanging modifier keys to use it:
- GlobalCommandsApiTest.GlobalCommand
- AppShimMenuControllerUITest, WindowCycling

Bug:  791457 
Change-Id: Id5eb2e04b2a20cf956cc6eee0c3cfe789b8b4375
Reviewed-on: https://chromium-review.googlesource.com/800073
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541372}
[modify] https://crrev.com/368836b81249724242221c47eff1106485b54da6/chrome/browser/extensions/extension_commands_global_registry_apitest.cc
[modify] https://crrev.com/368836b81249724242221c47eff1106485b54da6/chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_interactive_uitest.mm
[modify] https://crrev.com/368836b81249724242221c47eff1106485b54da6/chrome/test/base/OWNERS
[modify] https://crrev.com/368836b81249724242221c47eff1106485b54da6/chrome/test/base/interactive_test_utils.h
[modify] https://crrev.com/368836b81249724242221c47eff1106485b54da6/chrome/test/base/interactive_test_utils_mac.mm
[modify] https://crrev.com/368836b81249724242221c47eff1106485b54da6/chrome/test/base/interactive_ui_tests_main.cc
[modify] https://crrev.com/368836b81249724242221c47eff1106485b54da6/content/public/test/test_launcher.cc
[modify] https://crrev.com/368836b81249724242221c47eff1106485b54da6/content/public/test/test_launcher.h