NetworkServiceTestHelper is part of this target:

% gn refs out/debug //content/public/test/network_service_test_helper.cc
//content/test:test_support

And that is linked into the Content Shell Framework:

% gn path out/debug //content/test:test_support  //content/shell:content_shell_framework 
//content/shell:content_shell_framework --[public]-->
//content/shell:content_shell_lib --[private]-->
//content/test:test_support

So that means anything that uses Content Shell.app as the test runner should be okay from a network service perspective. From the internal thread, John wrote:

> We'd need this for all browser tests binaries, e.g. content_browsertests, browser_test, extensions_browsertests, components_browsertests, interactive_uitests (those are the ones that I know about).

Based on the above, content_browsertests should be okay. And components_browsertests uses content_shell as well. In https://chromium-review.googlesource.com/c/chromium/src/+/1134217 I made extensions_browsertests use the same binary for main and child processes. So it looks like the issue is limited to browser_tests and interactive_ui_tests, since both of those use Chromium.app as the test runner.

I got pretty far with this: https://chromium-review.googlesource.com/c/chromium/src/+/1193969/. But there are three tests that fail that I don't see a path forward on:

browser_tests:
ServiceProcessControlBrowserTest.LaunchAndReconnect

interactive_ui_tests:
AppShimInteractiveTest.Launch
AppShimInteractiveTest.ShowWindow

... they basically require the bundle structure to work.

I'm not familiar with the AppShim ones. Can you expand on why they can't work with browser_tests?

Why can't the service test work? Does it need to link some more data in the browser_tests binary on Mac?

I don't think it's about linking in code, but rather the lack of a bundle means there's no Info.plist that tells the system about the app. Processes behave differently if they're bundled or not, both by the system and in Chromium (e.g., base::mac::AmIBundled()).

I'm pretty sure the AppShim failures are related to the change I made here: https://chromium-review.googlesource.com/c/chromium/src/+/1193969/1/chrome/browser/app_controller_mac.mm. But we can't call chrome::GetVersionedDirectory() if the main executable isn't bundled. And if the AppShim doesn't have the right path to the binary then the test fails. I verified that this is the case by launching Chromium from my same build directory (which would set the right "last path" value) and the test then passed.

The service process test is similar. The production code is set up to point to a bundled application: https://cs.chromium.org/chromium/src/chrome/common/service_process_util_mac.mm?l=133&rcl=0527de47e7db6f6448483b88ff4be5da03e3698d

I'm not familiar with all this Mac stuff, so excuse the naive question which I'm sure you already thought of: so there's no way to add a bundle in the browser_tests binary?

I don't know how important these tests are; one option could be to create a new very limited test binary just for these on Mac? e.g. bundled_browser_tests or something similar?

+tapted in case he has any thoughts about the AppShim ones

The way browser_tests currently run is what makes them bundled. The issue is that they use the Chromium.app/Chromium Helper.app binaries, which are the same ones used in production. I did consider creating something like Browser Tests.app/Browser Tests Helper.app that link in test code (analogous to how Content Shell works), but that tends to be a bit of a maintenance burden.

I was also thinking something along those lines: a dedicated suite where we set up the test binary to use the bundled apps. I was wondering if this could be interactive_ui_tests? Or do you need to inject into the utility process for those too?

One other idea I had but have not yet explored: let browsertests continue to use the Chromium.app/Chromium Helper.app bundles, but the browser_test binary sets up its child processes to inject a shared library (either DYLD_INSERT_LIBRARIES or a new --load-test-helper-library). The shared lib would have the test code linked.

We could possibly move the app shim tests to their own (interactive-style) test target - they're likely going to get more interesting when we add features for PWAs.

Another option may be to pass an explicit path to the running Chrome bundle when the Chrome browser process triggers a shim launch. That may address what's happing in these tests, and even be more robust in the general case. (The last-run bundle path is still needed when the shim launches via Finder, but I don't think these failing tests are activating that path). However, that does mean we lose us a little bit of code coverage since we're skipping the launch-from-finder codepaths.

I would prefer that Mac works the same as the other platforms: e.g. the child binary is the same as the browser. This difference has prevented us from running the integration tests for the last year for network service, and will come up again for other services. Regarding interactive_ui_tests, yes we need the test code there.

Can you expand on why there's a maintenance burden for having browser tests helper.app?

Agree with #12 that we should keep the AppShim tests because they're the basis of upcoming Mac PWA support and may need to get more coverage. Not sure about the ServiceProcess test.

These test do directly launch the shim via LaunchPlatformAppWithCommandLineAndLaunchId(), so we could pass a path to either the socket or browser_tests (assuming it handled unbundled executables).

Re: #13: Mac fundamentally does not work the same as the other platforms in production, so I don't view matching browser_tests to the other platforms' model as necessarily the right thing to do.

The maintenance burden is around having to duplicate several hundred lines of build config, bundle artifacts, and a new exe_main.c file -- all of which would live parallel to the ones for Chromium.app and Content Shell.app. The CL I linked in #2 removed one example of such a shell app (for extensions_browsertests), but the one for a //chrome-level browser_tests would be more complex because there are more bundled resources/dependencies. Basically, we'd have to have a bunch of targets that would be almost the same as chrome/BUILD.gn lines 567-1342. Maybe we could use GN templates to make it simpler, but the level of indirection may itself be complicated too.

I'm still exploring the options though, so I haven't yet settled the optimal way to fix this. E.g., there may be a way to have the browser-process browser_tests pretend it's bundled, while still launching naked browser_tests child processes.

PS2 on https://chromium-review.googlesource.com/c/chromium/src/+/1193969 does what I noted at the end of #14: fake out the browser process to make it think it's bundled, but launch child processes directly as browser_tests. It's both a smaller change and all the tests seem to pass, so that's nice. Dry-running it now.

Thanks!

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

commit be983c8b8d2f58b9a70d9407feea02b5aba823c5
Author: Robert Sesek <rsesek@chromium.org>
Date: Thu Aug 30 19:31:37 2018

[Mac] In browser tests, do not use a bundled child process executable.

Previously, browser test binaries (e.g. browser_tests, interactive_ui_tests,
etc.) set up paths so that child processes would use Chromium Helper.app, which
matches the production process launching path. However this prevents the child
process from linking in test-only code, because it is a distinct binary from
the main test executable that acts as the browser process.

This change makes it so that the child process binary is the same as the main
test binary, which matches how other platforms behave.

Bug:  877992 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Ic04f0d47726be89d953ea227b8a104dc7bb88c0f
Reviewed-on: https://chromium-review.googlesource.com/1193969
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587705}
[modify] https://crrev.com/be983c8b8d2f58b9a70d9407feea02b5aba823c5/chrome/test/BUILD.gn
[modify] https://crrev.com/be983c8b8d2f58b9a70d9407feea02b5aba823c5/chrome/test/base/chrome_test_launcher.cc
[modify] https://crrev.com/be983c8b8d2f58b9a70d9407feea02b5aba823c5/chrome/test/base/in_process_browser_test.cc

I think the CL in #17 is incomplete. Some code paths can be taken to re-override CHILD_PROCESS_EXE which means that some tests use browser_tests as the child and others go back to Chromium Helper. I can fix that, though.

I think https://chromium-review.googlesource.com/c/chromium/src/+/1198306 should do it. Also reenabling the NetworkContextConfigurationBrowserTest for verification.

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

commit 81ea450944274c8574d4c3ee6c6adca9624e8a83
Author: Robert Sesek <rsesek@chromium.org>
Date: Tue Sep 04 15:12:18 2018

[Mac] Really make sure browser tests do not use a bundled child process executable.

The change in be983c8b8d2f58b9a70d9407feea02b5aba823c5 was incomplete
because calls into SetUpBundleOverrides() would re-override the
CHILD_PROCESS_EXE path. This moves the SetUpBundleOverrides() into ChromeMain
rather than in various delegate methods, so that it is not called in tests.

This requires allowing the crashpad_handler to be started from the Framework
that is not bundled in a .app, so the rpath is adjusted in the component build.

Finally this enables network service browser_tests, which were previously
disabled on Mac, to verify the fix.

Bug:  877992 
Test: browser_tests --gtest_filter=*NetworkContextConfigurationBrowserTest*
Change-Id: If88a8dd13b9b7b0500d08a180658597ae6f5997d
Reviewed-on: https://chromium-review.googlesource.com/1198306
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588516}
[modify] https://crrev.com/81ea450944274c8574d4c3ee6c6adca9624e8a83/chrome/app/chrome_main.cc
[modify] https://crrev.com/81ea450944274c8574d4c3ee6c6adca9624e8a83/chrome/app/chrome_main_delegate.cc
[modify] https://crrev.com/81ea450944274c8574d4c3ee6c6adca9624e8a83/chrome/browser/net/chrome_network_service_browsertest.cc
[modify] https://crrev.com/81ea450944274c8574d4c3ee6c6adca9624e8a83/chrome/browser/net/network_context_configuration_browsertest.cc
[modify] https://crrev.com/81ea450944274c8574d4c3ee6c6adca9624e8a83/chrome/browser/service_process/service_process_control_browsertest.cc
[modify] https://crrev.com/81ea450944274c8574d4c3ee6c6adca9624e8a83/third_party/crashpad/README.chromium
[modify] https://crrev.com/81ea450944274c8574d4c3ee6c6adca9624e8a83/third_party/crashpad/crashpad/handler/BUILD.gn

 Issue 843324  has been merged into this issue.