Why do we have content/public/test/ ?

[Dana Jansens]

Hello,

I am wondering why we have separate content/public/test/ vs content/test/ layers in the code. They are both part of the //content/test:test_support build target, so any test that depends on the content/public/ headers also gains access to the content/test/ ones, and we don't seem to have a problem with that. For example:

- content/shell/ uses content/test/ for test-bits while being built only on the public content API.

- There exist unit tests that include content/test/* from outside of content.

Why I ask:

I would like to write a test using a faked RenderWidgetHostView, I can do that with RenderViewHostTestHarness (in content/public/test/) which installs a TestRenderWidgetHostFactory among others such that things don't explode.

However, the TestRenderWidgetHostView is defined in content/test/ (even though the TestRenderWidgetHostViewFactory type is exposed in the public/ header). So my unit test will have to downcast the RenderWidgetHostView to a TestRenderWidgetHostView in order to observe what was called (I want to see what was passed to SetCursor()).

I was going to add a helper on RenderViewHostTestHarness that did that downcast, but then realized the TestRenderWidgetHostView is not in content/public/test/. My choices become:

1. Include content/test/ things in content/public/test/ to vend TestRenderWidgetHostView* since that is what RenderViewHostTestHarness provides.

2. Forward-decl TestRenderWidgetHostView and force any unit tests using RenderViewHostTestHarness to go include the content/test/ header.

3. Write my test (in content/) to do the downcast to TestRenderWidgetHostView* itself, which means the test relies on the implementation of RenderViewHostTestHarness in a very not-typesafe way.

4. Move TestRenderWidgetHostView to content/public/test/ since it's part of RenderViewHostTestHarness.

5. Don't use RenderViewHostTestHarness.. write my own RenderWidgetHostFactory and RenderWidgetHostView subclass.

Thoughts?

Thanks,

Dana

[John Abd-El-Malek]

On Fri, Nov 6, 2020 at 11:47 AM 'Dana Jansens' via content-owners <content...@chromium.org> wrote:

Hello,

I am wondering why we have separate content/public/test/ vs content/test/ layers in the code.

It was a design decision that tests shouldn't have access to content/ internals. Originally we didn't have content/public/test, just content/test. But then content/test headers would include stuff in content/browser and then chrome/ test code would be accessing internal content headers indirectly.

 

They are both part of the //content/test:test_support build target, so any test that depends on the content/public/ headers also gains access to the content/test/ ones,

We've been depending on DEPS rules blocking this. As you note below though sometimes content owners inadvertently +1 a cl that included this. We can fix this by splitting the targets; although we still have a situation where a content owner would inadvertently allow an external target to depend on the internal test target.

 

and we don't seem to have a problem with that. For example:

- content/shell/ uses content/test/ for test-bits while being built only on the public content API.

I think for content/shell it's just for web runner stuff right? I believe that used to only use public headers using the template magic, but we decided that's unnecessary which is why it now just directly includes content/test.

 

 

- There exist unit tests that include content/test/* from outside of content.

We should fix these..

 

Why I ask:

I would like to write a test using a faked RenderWidgetHostView, I can do that with RenderViewHostTestHarness (in content/public/test/) which installs a TestRenderWidgetHostFactory among others such that things don't explode.

However, the TestRenderWidgetHostView is defined in content/test/ (even though the TestRenderWidgetHostViewFactory type is exposed in the public/ header). So my unit test will have to downcast the RenderWidgetHostView to a TestRenderWidgetHostView in order to observe what was called (I want to see what was passed to SetCursor()).

I was going to add a helper on RenderViewHostTestHarness that did that downcast, but then realized the TestRenderWidgetHostView is not in content/public/test/. My choices become:

1. Include content/test/ things in content/public/test/ to vend TestRenderWidgetHostView* since that is what RenderViewHostTestHarness provides.

2. Forward-decl TestRenderWidgetHostView and force any unit tests using RenderViewHostTestHarness to go include the content/test/ header.

3. Write my test (in content/) to do the downcast to TestRenderWidgetHostView* itself, which means the test relies on the implementation of RenderViewHostTestHarness in a very not-typesafe way.

4. Move TestRenderWidgetHostView to content/public/test/ since it's part of RenderViewHostTestHarness.

5. Don't use RenderViewHostTestHarness.. write my own RenderWidgetHostFactory and RenderWidgetHostView subclass.

Thoughts?

Thanks,

Dana

--
You received this message because you are subscribed to the Google Groups "content-owners" group.
To unsubscribe from this group and stop receiving emails from it, send an email to content-owner...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAHtyhaRTOGUhKjEZ4TMYU%3D4i%2BiE1VvSHtfwCOT3tkiY9ziAZsQ%40mail.gmail.com.

[Jochen Eisinger]

On Fri, Nov 6, 2020 at 9:58 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Fri, Nov 6, 2020 at 11:47 AM 'Dana Jansens' via content-owners <content...@chromium.org> wrote:

Hello,

I am wondering why we have separate content/public/test/ vs content/test/ layers in the code.

It was a design decision that tests shouldn't have access to content/ internals. Originally we didn't have content/public/test, just content/test. But then content/test headers would include stuff in content/browser and then chrome/ test code would be accessing internal content headers indirectly.

 

They are both part of the //content/test:test_support build target, so any test that depends on the content/public/ headers also gains access to the content/test/ ones,

We've been depending on DEPS rules blocking this. As you note below though sometimes content owners inadvertently +1 a cl that included this. We can fix this by splitting the targets; although we still have a situation where a content owner would inadvertently allow an external target to depend on the internal test target.

 

and we don't seem to have a problem with that. For example:

- content/shell/ uses content/test/ for test-bits while being built only on the public content API.

I think for content/shell it's just for web runner stuff right? I believe that used to only use public headers using the template magic, but we decided that's unnecessary which is why it now just directly includes content/test.

Yeah, initially, content_shell only depended on content/public but we had this strange template stuff going on. Now we have a cleaner object hierarchy, but a layering violation.

 

 

 

- There exist unit tests that include content/test/* from outside of content.

We should fix these..

 

Why I ask:

I would like to write a test using a faked RenderWidgetHostView, I can do that with RenderViewHostTestHarness (in content/public/test/) which installs a TestRenderWidgetHostFactory among others such that things don't explode.

However, the TestRenderWidgetHostView is defined in content/test/ (even though the TestRenderWidgetHostViewFactory type is exposed in the public/ header). So my unit test will have to downcast the RenderWidgetHostView to a TestRenderWidgetHostView in order to observe what was called (I want to see what was passed to SetCursor()).

I was going to add a helper on RenderViewHostTestHarness that did that downcast, but then realized the TestRenderWidgetHostView is not in content/public/test/. My choices become:

1. Include content/test/ things in content/public/test/ to vend TestRenderWidgetHostView* since that is what RenderViewHostTestHarness provides.

2. Forward-decl TestRenderWidgetHostView and force any unit tests using RenderViewHostTestHarness to go include the content/test/ header.

3. Write my test (in content/) to do the downcast to TestRenderWidgetHostView* itself, which means the test relies on the implementation of RenderViewHostTestHarness in a very not-typesafe way.

4. Move TestRenderWidgetHostView to content/public/test/ since it's part of RenderViewHostTestHarness.

5. Don't use RenderViewHostTestHarness.. write my own RenderWidgetHostFactory and RenderWidgetHostView subclass.

Thoughts?

Thanks,

Dana

--
You received this message because you are subscribed to the Google Groups "content-owners" group.
To unsubscribe from this group and stop receiving emails from it, send an email to content-owner...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAHtyhaRTOGUhKjEZ4TMYU%3D4i%2BiE1VvSHtfwCOT3tkiY9ziAZsQ%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "content-owners" group.
To unsubscribe from this group and stop receiving emails from it, send an email to content-owner...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CALhVsw01bZV%2By-iRMz1rbXoVKHbuHqnj2WDGFRzVL6K1AYs8iw%40mail.gmail.com.

[Dana Jansens]

On Fri, Nov 6, 2020 at 3:58 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Fri, Nov 6, 2020 at 11:47 AM 'Dana Jansens' via content-owners <content...@chromium.org> wrote:

Hello,

I am wondering why we have separate content/public/test/ vs content/test/ layers in the code.

It was a design decision that tests shouldn't have access to content/ internals. Originally we didn't have content/public/test, just content/test. But then content/test headers would include stuff in content/browser and then chrome/ test code would be accessing internal content headers indirectly.

Thanks for explaining! It sounds to me then that TestRenderFrame/TestRenderWidgetHostView and friends should probably not be in content/public/ then, but it would be nice for tests written inside content to not have to downcast themselves. Maybe content_unittests could depend on //content_implementation and we could expose the types behind that?

Would it make sense to have a more type-safe API for content tests that shares the public test framework, but allows them to more deeply observe/interact with the content internal things?

They are both part of the //content/test:test_support build target, so any test that depends on the content/public/ headers also gains access to the content/test/ ones,

We've been depending on DEPS rules blocking this. As you note below though sometimes content owners inadvertently +1 a cl that included this. We can fix this by splitting the targets; although we still have a situation where a content owner would inadvertently allow an external target to depend on the internal test target.

We could use GN visibility to prevent that the way we prevent other content/ stuff from getting deps'd in by accident.

 

 

and we don't seem to have a problem with that. For example:

- content/shell/ uses content/test/ for test-bits while being built only on the public content API.

I think for content/shell it's just for web runner stuff right? I believe that used to only use public headers using the template magic, but we decided that's unnecessary which is why it now just directly includes content/test.

That was one use historically, but content_shell also has a bunch of browsertest support code too, some using //content/test. I'd love to move that to another GN target that browsertests include, and have a CL kind of going in that direction but there's more to do.

The web-test code is now moved out of shell into //content/web_test/ and is depended on only by //content/shell/app to inject it, so happily it's not (or less of) a concern now (we should maybe have a different web-test-content-shell-app and binary but anyhow...).

 

 

 

- There exist unit tests that include content/test/* from outside of content.

We should fix these..

- Looks like TestContentBrowserClient could move to content/public/test/

- The blink tests make use of a subclass of (content-private) BlinkPlatformImpl which seems ok, but the creation is through content/public/test/blink_test_environment.h so the mutation of it probably should be too.

The least clear:

- TestWebContents probably shouldn't (depends on WebContentsImpl). It's unclear to me if ui/views/controls/webview/web_dialog_view_unittest.cc could just stop using that.

[Dana Jansens]

On Fri, Nov 6, 2020 at 4:17 PM Dana Jansens <dan...@google.com> wrote:

On Fri, Nov 6, 2020 at 3:58 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Fri, Nov 6, 2020 at 11:47 AM 'Dana Jansens' via content-owners <content...@chromium.org> wrote:

Hello,

I am wondering why we have separate content/public/test/ vs content/test/ layers in the code.

It was a design decision that tests shouldn't have access to content/ internals. Originally we didn't have content/public/test, just content/test. But then content/test headers would include stuff in content/browser and then chrome/ test code would be accessing internal content headers indirectly.

Thanks for explaining! It sounds to me then that TestRenderFrame/TestRenderWidgetHostView and friends should probably not be in content/public/ then, but it would be nice for tests written inside content to not have to downcast themselves. Maybe content_unittests could depend on //content_implementation and we could expose the types behind that?

Would it make sense to have a more type-safe API for content tests that shares the public test framework, but allows them to more deeply observe/interact with the content internal things?

They are both part of the //content/test:test_support build target, so any test that depends on the content/public/ headers also gains access to the content/test/ ones,

We've been depending on DEPS rules blocking this. As you note below though sometimes content owners inadvertently +1 a cl that included this. We can fix this by splitting the targets; although we still have a situation where a content owner would inadvertently allow an external target to depend on the internal test target.

We could use GN visibility to prevent that the way we prevent other content/ stuff from getting deps'd in by accident.

 

 

and we don't seem to have a problem with that. For example:

- content/shell/ uses content/test/ for test-bits while being built only on the public content API.

I think for content/shell it's just for web runner stuff right? I believe that used to only use public headers using the template magic, but we decided that's unnecessary which is why it now just directly includes content/test.

That was one use historically, but content_shell also has a bunch of browsertest support code too, some using //content/test. I'd love to move that to another GN target that browsertests include, and have a CL kind of going in that direction but there's more to do.

The web-test code is now moved out of shell into //content/web_test/ and is depended on only by //content/shell/app to inject it, so happily it's not (or less of) a concern now (we should maybe have a different web-test-content-shell-app and binary but anyhow...).

 

 

 

- There exist unit tests that include content/test/* from outside of content.

We should fix these..

- Looks like TestContentBrowserClient could move to content/public/test/

- The blink tests make use of a subclass of (content-private) BlinkPlatformImpl which seems ok, but the creation is through content/public/test/blink_test_environment.h so the mutation of it probably should be too.

The least clear:

- TestWebContents probably shouldn't (depends on WebContentsImpl). It's unclear to me if ui/views/controls/webview/web_dialog_view_unittest.cc could just stop using that.

[oops moved this so it got less clear - probably shouldn't move to public/ I meant to say]

[John Abd-El-Malek]

On Fri, Nov 6, 2020 at 1:18 PM Dana Jansens <dan...@google.com> wrote:

On Fri, Nov 6, 2020 at 4:17 PM Dana Jansens <dan...@google.com> wrote:

On Fri, Nov 6, 2020 at 3:58 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Fri, Nov 6, 2020 at 11:47 AM 'Dana Jansens' via content-owners <content...@chromium.org> wrote:

Hello,

I am wondering why we have separate content/public/test/ vs content/test/ layers in the code.

It was a design decision that tests shouldn't have access to content/ internals. Originally we didn't have content/public/test, just content/test. But then content/test headers would include stuff in content/browser and then chrome/ test code would be accessing internal content headers indirectly.

Thanks for explaining! It sounds to me then that TestRenderFrame/TestRenderWidgetHostView and friends should probably not be in content/public/ then,

Yes I didn't realize they're not used outside content (now?). Test helpers should only be in content/public/test if they're used outside of content.

 

but it would be nice for tests written inside content to not have to downcast themselves. Maybe content_unittests could depend on //content_implementation and we could expose the types behind that?

I'd be very surprised if content_unittests doesn't depend on content implementation given all the tests that interact with internal classes? (I didn't check build rules)

 

Would it make sense to have a more type-safe API for content tests that shares the public test framework, but allows them to more deeply observe/interact with the content internal things?

I'm not sure what we would gain through this extra effort? This may just be a personal preference, but from my pov casting in unittests is fine.

 

They are both part of the //content/test:test_support build target, so any test that depends on the content/public/ headers also gains access to the content/test/ ones,

We've been depending on DEPS rules blocking this. As you note below though sometimes content owners inadvertently +1 a cl that included this. We can fix this by splitting the targets; although we still have a situation where a content owner would inadvertently allow an external target to depend on the internal test target.

We could use GN visibility to prevent that the way we prevent other content/ stuff from getting deps'd in by accident.

Right, what I was trying to say is that someone could incorrectly approve a cl allowing chrome/ target to depend on internal content target, the same way they could incorrectly approve a DEPS change. Note that a deps change in chrome/ to depend on content/browser or content/test requires a content owner lgtm. But probably the GN visibility approach is less error prone, because the place to edit would be in one place only and we can put a big warning in all caps.

 

 

 

and we don't seem to have a problem with that. For example:

- content/shell/ uses content/test/ for test-bits while being built only on the public content API.

I think for content/shell it's just for web runner stuff right? I believe that used to only use public headers using the template magic, but we decided that's unnecessary which is why it now just directly includes content/test.

That was one use historically, but content_shell also has a bunch of browsertest support code too, some using //content/test.

Ah, got it. This link surprises me, since the mock object seems to be applied for both content_browsertests, web test runner, and content_shell. The latter shouldn't be using mock objects.

 

I'd love to move that to another GN target that browsertests include, and have a CL kind of going in that direction but there's more to do.

The web-test code is now moved out of shell into //content/web_test/ and is depended on only by //content/shell/app to inject it, so happily it's not (or less of) a concern now (we should maybe have a different web-test-content-shell-app and binary but anyhow...).

 

 

 

- There exist unit tests that include content/test/* from outside of content.

We should fix these..

- Looks like TestContentBrowserClient could move to content/public/test/

- The blink tests make use of a subclass of (content-private) BlinkPlatformImpl which seems ok, but the creation is through content/public/test/blink_test_environment.h so the mutation of it probably should be too.

The least clear:

- TestWebContents probably shouldn't (depends on WebContentsImpl). It's unclear to me if ui/views/controls/webview/web_dialog_view_unittest.cc could just stop using that.

[oops moved this so it got less clear - probably shouldn't move to public/ I meant to say]

It seems we could fix that test just by adding a public constructor for TestWebContents?

[Dana Jansens]

On Fri, Nov 6, 2020 at 5:40 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Fri, Nov 6, 2020 at 1:18 PM Dana Jansens <dan...@google.com> wrote:

On Fri, Nov 6, 2020 at 4:17 PM Dana Jansens <dan...@google.com> wrote:

On Fri, Nov 6, 2020 at 3:58 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Fri, Nov 6, 2020 at 11:47 AM 'Dana Jansens' via content-owners <content...@chromium.org> wrote:

Hello,

I am wondering why we have separate content/public/test/ vs content/test/ layers in the code.

It was a design decision that tests shouldn't have access to content/ internals. Originally we didn't have content/public/test, just content/test. But then content/test headers would include stuff in content/browser and then chrome/ test code would be accessing internal content headers indirectly.

Thanks for explaining! It sounds to me then that TestRenderFrame/TestRenderWidgetHostView and friends should probably not be in content/public/ then,

Yes I didn't realize they're not used outside content (now?). Test helpers should only be in content/public/test if they're used outside of content.

They're used by any tests built on RenderViewHostTestEnabler or RenderViewHostTestHarness, which are in //content/public/test/ and widely used, but their types are hidden. Because the types are hidden, tests _inside content_ also don't get to see them, or change them to other types to inject their own behaviours/.

 

 

but it would be nice for tests written inside content to not have to downcast themselves. Maybe content_unittests could depend on //content_implementation and we could expose the types behind that?

I'd be very surprised if content_unittests doesn't depend on content implementation given all the tests that interact with internal classes? (I didn't check build rules)

Right, it doesn't today. Maybe it would cause similar linking issues as BrowserTestBase, but it'd be nice to provide a nicer API for content tests than just what we give to public/ IMO.

 

 

Would it make sense to have a more type-safe API for content tests that shares the public test framework, but allows them to more deeply observe/interact with the content internal things?

I'm not sure what we would gain through this extra effort? This may just be a personal preference, but from my pov casting in unittests is fine.

It may be moot if no one is mucking with the RenderViewHostTestHarness and friends, but the type of RenderFrameHost etc that it constructs is an implementation detail of RenderViewHostTestHarness, so tests casting things is not typesafe and over time could break in confusing ways. It's not the type of API we would accept in prod, but it's all we provide for test-writing.

They are both part of the //content/test:test_support build target, so any test that depends on the content/public/ headers also gains access to the content/test/ ones,

We've been depending on DEPS rules blocking this. As you note below though sometimes content owners inadvertently +1 a cl that included this. We can fix this by splitting the targets; although we still have a situation where a content owner would inadvertently allow an external target to depend on the internal test target.

We could use GN visibility to prevent that the way we prevent other content/ stuff from getting deps'd in by accident.

Right, what I was trying to say is that someone could incorrectly approve a cl allowing chrome/ target to depend on internal content target, the same way they could incorrectly approve a DEPS change. Note that a deps change in chrome/ to depend on content/browser or content/test requires a content owner lgtm. But probably the GN visibility approach is less error prone, because the place to edit would be in one place only and we can put a big warning in all caps.

 

 

 

and we don't seem to have a problem with that. For example:

- content/shell/ uses content/test/ for test-bits while being built only on the public content API.

I think for content/shell it's just for web runner stuff right? I believe that used to only use public headers using the template magic, but we decided that's unnecessary which is why it now just directly includes content/test.

That was one use historically, but content_shell also has a bunch of browsertest support code too, some using //content/test.

Ah, got it. This link surprises me, since the mock object seems to be applied for both content_browsertests, web test runner, and content_shell. The latter shouldn't be using mock objects.

There is more test-only code in content_shell for content_browsertests in ShellContentUtilityClient as well. Likely other places I'm less aware of too.

 

 

I'd love to move that to another GN target that browsertests include, and have a CL kind of going in that direction but there's more to do.

The web-test code is now moved out of shell into //content/web_test/ and is depended on only by //content/shell/app to inject it, so happily it's not (or less of) a concern now (we should maybe have a different web-test-content-shell-app and binary but anyhow...).

 

 

 

- There exist unit tests that include content/test/* from outside of content.

We should fix these..

- Looks like TestContentBrowserClient could move to content/public/test/

- The blink tests make use of a subclass of (content-private) BlinkPlatformImpl which seems ok, but the creation is through content/public/test/blink_test_environment.h so the mutation of it probably should be too.

The least clear:

- TestWebContents probably shouldn't (depends on WebContentsImpl). It's unclear to me if ui/views/controls/webview/web_dialog_view_unittest.cc could just stop using that.

[oops moved this so it got less clear - probably shouldn't move to public/ I meant to say]

It seems we could fix that test just by adding a public constructor for TestWebContents?

The unit test is outside content, but TestWebContents subclasses WebContentsImpl and is in content/test/, not content/public/test/. The test is able to construct the type but it should be using WebContentsImpl instead based on where the test is written.. it's not obvious to me why it chose not to.

[John Abd-El-Malek]

On Fri, Nov 6, 2020 at 2:54 PM Dana Jansens <dan...@google.com> wrote:

On Fri, Nov 6, 2020 at 5:40 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Fri, Nov 6, 2020 at 1:18 PM Dana Jansens <dan...@google.com> wrote:

On Fri, Nov 6, 2020 at 4:17 PM Dana Jansens <dan...@google.com> wrote:

On Fri, Nov 6, 2020 at 3:58 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Fri, Nov 6, 2020 at 11:47 AM 'Dana Jansens' via content-owners <content...@chromium.org> wrote:

Hello,

I am wondering why we have separate content/public/test/ vs content/test/ layers in the code.

It was a design decision that tests shouldn't have access to content/ internals. Originally we didn't have content/public/test, just content/test. But then content/test headers would include stuff in content/browser and then chrome/ test code would be accessing internal content headers indirectly.

Thanks for explaining! It sounds to me then that TestRenderFrame/TestRenderWidgetHostView and friends should probably not be in content/public/ then,

Yes I didn't realize they're not used outside content (now?). Test helpers should only be in content/public/test if they're used outside of content.

They're used by any tests built on RenderViewHostTestEnabler or RenderViewHostTestHarness, which are in //content/public/test/ and widely used, but their types are hidden. Because the types are hidden, tests _inside content_ also don't get to see them, or change them to other types to inject their own behaviours/.

Do any tests inside content need to be able to change them to other types? It sounds like not, since they're not exposed. If so we can move them to content/test and I think not worry about if tests in content might want to interact with them in the future. 

 

 

but it would be nice for tests written inside content to not have to downcast themselves. Maybe content_unittests could depend on //content_implementation and we could expose the types behind that?

I'd be very surprised if content_unittests doesn't depend on content implementation given all the tests that interact with internal classes? (I didn't check build rules)

Right, it doesn't today.

I'm still not following, I see plenty of unittests inside conde depending on implementations inside content/. How do they do that without depending on content_unittests?

 

Maybe it would cause similar linking issues as BrowserTestBase, but it'd be nice to provide a nicer API for content tests than just what we give to public/ IMO.

 

 

Would it make sense to have a more type-safe API for content tests that shares the public test framework, but allows them to more deeply observe/interact with the content internal things?

I'm not sure what we would gain through this extra effort? This may just be a personal preference, but from my pov casting in unittests is fine.

It may be moot if no one is mucking with the RenderViewHostTestHarness and friends, but the type of RenderFrameHost etc that it constructs is an implementation detail of RenderViewHostTestHarness, so tests casting things is not typesafe and over time could break in confusing ways. It's not the type of API we would accept in prod, but it's all we provide for test-writing.

IMO RenderViewHostTestHarness and other large harnesses are more trouble than they're worth, because they're for unittests but have so many other classes involved. Some of the other classes are mocks while some are production classes. I and others have found them a pain when changing content implementation details. Pragmatically it's hard to remove them, but I wouldn't want them to introduce extra layers or complexity.

 

They are both part of the //content/test:test_support build target, so any test that depends on the content/public/ headers also gains access to the content/test/ ones,

We've been depending on DEPS rules blocking this. As you note below though sometimes content owners inadvertently +1 a cl that included this. We can fix this by splitting the targets; although we still have a situation where a content owner would inadvertently allow an external target to depend on the internal test target.

We could use GN visibility to prevent that the way we prevent other content/ stuff from getting deps'd in by accident.

Right, what I was trying to say is that someone could incorrectly approve a cl allowing chrome/ target to depend on internal content target, the same way they could incorrectly approve a DEPS change. Note that a deps change in chrome/ to depend on content/browser or content/test requires a content owner lgtm. But probably the GN visibility approach is less error prone, because the place to edit would be in one place only and we can put a big warning in all caps.

 

 

 

and we don't seem to have a problem with that. For example:

- content/shell/ uses content/test/ for test-bits while being built only on the public content API.

I think for content/shell it's just for web runner stuff right? I believe that used to only use public headers using the template magic, but we decided that's unnecessary which is why it now just directly includes content/test.

That was one use historically, but content_shell also has a bunch of browsertest support code too, some using //content/test.

Ah, got it. This link surprises me, since the mock object seems to be applied for both content_browsertests, web test runner, and content_shell. The latter shouldn't be using mock objects.

There is more test-only code in content_shell for content_browsertests in ShellContentUtilityClient as well. Likely other places I'm less aware of too.

Got it, seems like we could remove that if we had a subclass of SCUC for the browsertest target. Probably on the list of cleanups this is low.