Add the most minimal possible ContentBrowserTest in Rust. [chromium/src : main]
danakj (Gerrit)
Attention is currently required from: Adrian Taylor, Charlie Reis.
1 comment:
Patchset:
adetaylor: overall + rustiness
creis: owners
To view, visit change 3645146. To unsubscribe, or for help writing mail filters, visit settings.
Charlie Reis (Gerrit)
Attention is currently required from: Adrian Taylor, danakj.
Charlie Reis removed null from this change.
Add the most minimal possible ContentBrowserTest in Rust.
The test is just an empty body so far. But it sets up the instructure
to inherit from content::ContentBrowserTest for the Gtest test suite,
and pass the Rust type for it to the test body.
We shouldn't mix public and private content APIs in a single crate as
that would allow public-API consumers to also use private APIs, unless
we introduce some way to restrict that.
So for now, we have created separate "rust_public_foo" GN targets for
//content/public/test things. This is discussed in detail in
https://bugs.chromium.org/p/chromium/issues/detail?id=1325336.
R=adet...@chromium.org, cr...@chromium.org
Bug: 1305396, 1325336
Change-Id: Ic5df4b0ed5313d53e7e441f3800c0d89ee1c5f43
Cq-Include-Trybots: luci.chromium.try:linux-rust-x64-rel,linux-rust-x64-dbg,android-rust-arm-rel,android-rust-arm-dbg
---
A content/browser/smoke_rust_browsertest.rs
M content/public/test/browser_test_base.h
A content/public/test/browser_test_macro.rs
M content/public/test/content_browser_test.cc
A content/public/test/content_browser_test.rs
A content/public/test/content_public_browsertest_support.rs
A content/public/test/content_public_test_support.rs
M content/test/BUILD.gn
8 files changed, 238 insertions(+), 4 deletions(-)
Charlie Reis (Gerrit)
Attention is currently required from: Adrian Taylor, danakj.
1 comment:
Patchset:
Nice. I haven't reviewed many Rust additions, and I don't see any other uses of RUST_ENABLED in content/ (or outside build/). I think I remember hearing there are other test examples somewhere, though? Is there a precedent I can look at?
In general, it seems like this is in line with https://chromium.googlesource.com/chromium/src/+/main/docs/security/rust-toolchain.md#guidelines, since it's a test-only change (non-shipping) that only affects things when enable_rust = true is in GN. Is that correct?
Also, looks like the bots are unhappy at the moment. :)
To view, visit change 3645146. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Adrian Taylor, Charlie Reis.
1 comment:
Patchset:
Nice. […]
Yes, that is correct. The bots are unhappy with the parent change, and I've rebased it to make them happier.
This is our first foray outside of //base and //testing. But you can see examples in both of //base/BUILD.gn and //testing/rust_gtest_interop for other Rust/C++ stuff.
To view, visit change 3645146. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Adrian Taylor, Charlie Reis, danakj.
danakj uploaded patch set #9 to this change.
Add the most minimal possible ContentBrowserTest in Rust.
The test is just an empty body so far. But it sets up the instructure
to inherit from content::ContentBrowserTest for the Gtest test suite,
and pass the Rust type for it to the test body.
We shouldn't mix public and private content APIs in a single crate as
that would allow public-API consumers to also use private APIs, unless
we introduce some way to restrict that.
So for now, we have created separate "rust_public_foo" GN targets
for //content/public/test things. This is discussed in detail in
https://crbug.com/1325336.
R=adet...@chromium.org, cr...@chromium.org
Bug: 1305396, 1325336
Change-Id: Ic5df4b0ed5313d53e7e441f3800c0d89ee1c5f43
Cq-Include-Trybots: luci.chromium.try:linux-rust-x64-rel,linux-rust-x64-dbg,android-rust-arm-rel,android-rust-arm-dbg
---
A content/browser/smoke_rust_browsertest.rs
M content/public/test/browser_test_base.h
A content/public/test/browser_test_macro.rs
M content/public/test/content_browser_test.cc
A content/public/test/content_browser_test.rs
A content/public/test/content_public_browsertest_support.rs
A content/public/test/content_public_test_support.rs
M content/test/BUILD.gn
8 files changed, 238 insertions(+), 4 deletions(-)
To view, visit change 3645146. To unsubscribe, or for help writing mail filters, visit settings.
Charlie Reis (Gerrit)
Attention is currently required from: Adrian Taylor, danakj, Łukasz Anforowicz.
Charlie Reis would like Łukasz Anforowicz to review this change authored by danakj.
Charlie Reis (Gerrit)
Attention is currently required from: Adrian Taylor, danakj, Łukasz Anforowicz.
8 comments:
Commit Message:
Patch Set #9, Line 19:
https://crbug.com/1325336.I've skimmed this bug, but it looks beyond my skill to review, and seems like an important decision. Should we have someone with more GN (and/or Rust) knowledge weigh in on how to structure the public API and targets before landing this?
Patchset:
Yes, that is correct. […]
Thanks! I've done a first round and left some questions, largely because this is unfamiliar to me and I'm trying to learn. Apologies if any of it doesn't make sense. :)
I do think it's worth moving lukasza@ to be a full reviewer of this CL, given his reviews of some of the other Rust testing CLs (e.g., r987906). There's a lot of indirection with macros, etc, and I'm not sure I follow it all.
File content/browser/smoke_rust_browsertest.rs:
Patch Set #9, Line 11:
#[gtest_suite(ContentBrowserTest)]Can you help me understand what this means? What's the difference between gtest and gtest_suite? Is RustBrowserTest a general category of tests (like RustBrowserTestBase) as a kind of subclass of ContentBrowserTest, or is it the name of this particular smoke test class? If it's the latter, maybe (RustSmokeBrowserTest, Smoke) is a clearer example for other new tests to follow?
In other words, if I were to write a new test for navigation modeled on this file, would I say (RustBrowserTest, NavigateInPopup), or (RustNavigationBrowserTest, NavigateInPopup), or something else?
File content/public/test/browser_test_base.h:
Patch Set #9, Line 289:
#if defined(RUST_ENABLED)Does this belong directly in browser_test_base.h, or would it fit better in a new file like rust_browser_test_base.h (or some other existing file)?
Patch Set #9, Line 307:
class RustBrowserTestBase : public Subclass {Maybe I'm not used to thinking about it this way, but these names feel backward to me (i.e., having a "base" class extend something named Subclass).
Is this the resulting hierarchy?
- public ::testing::Test
- BrowserTestBase
- ContentBrowserTest
- FooTest
- RustBrowserTestBase<FooTest>
If so, maybe the last one should have a different name, like RustBrowserTestFixture<TestClass>, or some other word that suggests it's a framework for running the test and not a base class to be inherited from?
File content/public/test/content_browser_test.rs:
Patch Set #9, Line 1:
// Copyright 2022 The Chromium Authors. All rights reserved.If this isn't a "public" interface, is there any way we can put it in content/test instead of content/public/test?
File content/public/test/content_public_browsertest_support.rs:
Patch Set #9, Line 1:
// Copyright 2022 The Chromium Authors. All rights reserved.This seems to be the file included by content/browser/smoke_rust_browsertest.rs, right? If it's the main public interface for Rust browser tests in content/, can we include a high level comment about how to use it here?
File content/public/test/content_public_test_support.rs:
Patch Set #9, Line 1:
// Copyright 2022 The Chromium Authors. All rights reserved.I'm less clear what this interface is for. It appears to be used inside content_browser_test.rs, but is it more of an internal detail? Or will actual test files end up using it as well?
I'm wondering if this is something that doesn't need to be part of a "public" interface (whether that means putting it in content/test instead of content/public/test or removing "_public" from the name)?
To view, visit change 3645146. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Adrian Taylor, Charlie Reis, Łukasz Anforowicz.
7 comments:
Commit Message:
I've skimmed this bug, but it looks beyond my skill to review, and seems like an important decision. […]
I don't think it blocks us from landing this. We will need to make adjustments no matter what we end up doing, this is part of our experiment to uncover what sorts of things we do in fact need to do. The information gathered through this will feed back into the decisions we end up making.
Patchset:
Thanks Charlie, some thoughts below
File content/browser/smoke_rust_browsertest.rs:
Patch Set #9, Line 11:
#[gtest_suite(ContentBrowserTest)]
Can you help me understand what this means? What's the difference between gtest and gtest_suite? I […]
#[gtest] defines the function to be a test. It's like TEST() in C++.
#[gtest_suite] chooses the C++ class that acts as the test suite. It's like TEST_F() in C++, except acting as a modifier of #[gtest] here.
"RustBrowserTest.Smoke" will be the name of the test, they are just strings.
If you were writing another test you'd put whatever makes sense in the #[gtest] macro and you'd put ContentBrowserTest in the #[gtest_suite] macro (unless you were using a different base class like InProcessBrowserTest)
Here's docs: https://chromium.googlesource.com/chromium/src/+/main/testing/rust_gtest_interop/README.md
File content/public/test/browser_test_base.h:
Patch Set #9, Line 289:
#if defined(RUST_ENABLED)
Does this belong directly in browser_test_base. […]
This is a macro for subclasses of BrowserTestBase to be made usable from Rust. I think it makes sense as a part of this file.
Patch Set #9, Line 307:
class RustBrowserTestBase : public Subclass {
Maybe I'm not used to thinking about it this way, but these names feel backward to me (i.e. […]
The `Subclass` here is a subclass of BrowserTestBase.
testing::Test
BrowserTestBase
ContentBrowserTest
RustBrowserTestBase<ContentBrowserTest>It's refering to "BrowserTestBase" with its name here, it's a class providing Rust access to BrowserTestBase (and a Subclass of it).
Originally I had called it RustBrowserTest but that felt a bit wrong too. I'm not sure adding Fixture helps or not. WDYT with this additional context?
File content/public/test/content_browser_test.rs:
Patch Set #9, Line 1:
// Copyright 2022 The Chromium Authors. All rights reserved.
If this isn't a "public" interface, is there any way we can put it in content/test instead of conten […]
What do you mean that it isn't public? It would be used in the same public way that C++ ContentBrowserTest is used. Any content-based browser test would use it, written in content or elsewhere.
File content/public/test/content_public_browsertest_support.rs:
Patch Set #9, Line 1:
// Copyright 2022 The Chromium Authors. All rights reserved.
This seems to be the file included by content/browser/smoke_rust_browsertest. […]
This is the root (crate root) of the content_public_browsertest_support GN target. It defines what is publicly exported from the crate, but that's about all this file does.
smoke_rust_browsertest does depend on this crate, it imports the ContentBrowserType name from it. I agree it would be good to write some browser-test-specific docs somewhere. Would that be a good place for it? I've added it there.
This file would eventually grow to include all modules and define all public modules or types for the GN target. It could contain docs about "what is content_public_browsertest_support" if needed, but probably not the right place for class-specific stuff.
To view, visit change 3645146. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Adrian Taylor, Charlie Reis, Łukasz Anforowicz.
1 comment:
Commit Message:
I don't think it blocks us from landing this. […]
To be extra clear, I don't intend to make any decisions about what to do about this at the moment. I will just be gathering this as a consideration to help make higher level decisions.
To view, visit change 3645146. To unsubscribe, or for help writing mail filters, visit settings.
Adrian Taylor (Gerrit)
Attention is currently required from: Charlie Reis, danakj, Łukasz Anforowicz.
Patch set 10:Code-Review +1
2 comments:
Patchset:
LGTM from me but this is a very superficial review of the Rusty bits. I think the important parts to get right are around naming and source code layout, which you're engaging with Charlie about.
File content/browser/smoke_rust_browsertest.rs:
Patch Set #10, Line 12:
fn test(_bt: Pin<&mut ContentBrowserTest>) -> Result<(), Box<dyn Error>> {I wondered if you'd considered using https://docs.rs/anyhow/latest/anyhow/
To view, visit change 3645146. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Charlie Reis, Łukasz Anforowicz.
1 comment:
File content/browser/smoke_rust_browsertest.rs:
Patch Set #10, Line 12:
fn test(_bt: Pin<&mut ContentBrowserTest>) -> Result<(), Box<dyn Error>> {
I wondered if you'd considered using https://docs. […]
A while ago but not really yet. I think maybe doing so here would force it on all users of #[gtest] which I am not sure I like. It could be a way to expand the macro in the future if it can be done in parallel to normal errors.
To view, visit change 3645146. To unsubscribe, or for help writing mail filters, visit settings.
Charlie Reis (Gerrit)
Attention is currently required from: danakj, Łukasz Anforowicz.
11 comments:
Commit Message:
To be extra clear, I don't intend to make any decisions about what to do about this at the moment. […]
Thanks. Can you add something here in the CL description to make that clearer to others, like "This is discussed in detail in ..., and we plan to revisit the GN target design based on our findings"?
Commit Message:
Patch Set #10, Line 15:
we introduce some way to restrict that.To help me understand this paragraph, are there any examples of private APIs in this CL, which we have avoided putting in the "rust_public_foo" GN targets?
Patchset:
Thanks! More thoughts and questions as I get closer to understanding how this works.
File content/browser/smoke_rust_browsertest.rs:
Patch Set #9, Line 11:
#[gtest_suite(ContentBrowserTest)]
#[gtest] defines the function to be a test. It's like TEST() in C++. […]
Thanks, that helps. I think s/RustBrowserTest/SmokeRustBrowserTest/ would make that clearer, and be consistent with the filename? (That would have avoided my confusion about whether RustBrowserTest is a fixed name that has to be used for all browser tests written in Rust, like the other macros defined in this CL.)
File content/public/test/browser_test_base.h:
Patch Set #9, Line 289:
#if defined(RUST_ENABLED)
This is a macro for subclasses of BrowserTestBase to be made usable from Rust. […]
Ack
Patch Set #9, Line 307:
class RustBrowserTestBase : public Subclass {
The `Subclass` here is a subclass of BrowserTestBase. […]
Oh, thanks-- I was parsing it as "RustBrowserTest Base," but it sounds like it's actually "Rust BrowserTestBase."
I actually think RustBrowserTest<...> might convey that more clearly, keeping the "Base" part higher in the class hierarchy and not in the name of this class:
- public ::testing::Test
- BrowserTestBase
- ContentBrowserTest
- RustBrowserTest<ContentBrowserTest>
We can then make the class-level comment more explicit, like "This class provides Rust access to BrowserTestBase and its subclasses. It replaces..."
I'm still trying to think of a better name for the template parameter, to make it sound less like this class is extending its own subclass. Would RustBrowserTest<BrowserTestBaseSubclass> be too wordy?
File content/public/test/browser_test_macro.rs:
Patch Set #10, Line 1:
// Copyright 2022 The Chromium Authors. All rights reserved.Is this file part of the "public" interface as well? I don't see extern_browsertest_suite mentioned in the smoke test file, but I haven't quite figured out how these all fit together.
File content/public/test/content_browser_test.rs:
Patch Set #9, Line 1:
// Copyright 2022 The Chromium Authors. All rights reserved.
What do you mean that it isn't public? It would be used in the same public way that C++ ContentBrows […]
Ah, I clearly missed the key line of this file, since I thought smoke_rust_browsertest.rs was including what it needed from content_public_browsertest_support.rs and not from here. I guess it's both?
The comments you've added do help make it clearer that this is part of the public interface.
File content/public/test/content_browser_test.rs:
Patch Set #10, Line 19:
inherit usenit: Drop one of these words?
File content/public/test/content_public_browsertest_support.rs:
Patch Set #9, Line 1:
// Copyright 2022 The Chromium Authors. All rights reserved.
This is the root (crate root) of the content_public_browsertest_support GN target. […]
Thanks. Maybe just a comment summarizing what this file is will help others like me? (I don't know what a crate root is, but some description of how this file defines what's exported seems like it would be useful.)
File content/public/test/content_public_test_support.rs:
Patch Set #9, Line 1:
// Copyright 2022 The Chromium Authors. All rights reserved.
I'm less clear what this interface is for. It appears to be used inside content_browser_test. […]
As with the other cases, I'm mainly trying to draw a line between "what do people writing tests outside content/ need to have available to them" and "what code is implementing the internal support for those functions." I don't immediately see how smoke_rust_browsertest.cc directly depends on this file, but maybe it does?
To view, visit change 3645146. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: danakj, Łukasz Anforowicz.
danakj uploaded patch set #11 to this change.
Add the most minimal possible ContentBrowserTest in Rust.
The test is just an empty body so far. But it sets up the instructure
to inherit from content::ContentBrowserTest for the Gtest test suite,
and pass the Rust type for it to the test body.
We shouldn't mix public and private content APIs in a single crate as
that would allow public-API consumers to also use private APIs, unless
we introduce some way to restrict that.
So for now, we have created separate "rust_public_foo" GN targets
for //content/public/test things. This is discussed in detail in
https://crbug.com/1325336, and we will need to revisit how to structure
public vs private things between Rust and C++ based on the outcome
there.
R=adet...@chromium.org, cr...@chromium.org
Bug: 1305396, 1325336
Change-Id: Ic5df4b0ed5313d53e7e441f3800c0d89ee1c5f43
Cq-Include-Trybots: luci.chromium.try:linux-rust-x64-rel,linux-rust-x64-dbg,android-rust-arm-rel,android-rust-arm-dbg
---
A content/browser/smoke_rust_browsertest.rs
M content/public/test/browser_test_base.h
A content/public/test/browser_test_macro.rs
M content/public/test/content_browser_test.cc
A content/public/test/content_browser_test.rs
A content/public/test/content_public_browsertest_support.rs
A content/public/test/content_public_test_support.rs
M content/test/BUILD.gn
8 files changed, 253 insertions(+), 4 deletions(-)
To view, visit change 3645146. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Charlie Reis, Łukasz Anforowicz.
10 comments:
Commit Message:
Thanks. […]
Done
Commit Message:
Patch Set #10, Line 15:
we introduce some way to restrict that.
To help me understand this paragraph, are there any examples of private APIs in this CL, which we ha […]
No, I didn't need any private content stuff in the test.
But to help clarify. If I was to wrap a C++ class in //content/test, it could not go into the same crate. If it did, then code outside of content using that crate (to use a public thing like ContentBrowserTest) would also be able to use the private thing from //content/test.
Patchset:
PTAL
File content/browser/smoke_rust_browsertest.rs:
Patch Set #9, Line 11:
#[gtest_suite(ContentBrowserTest)]
Thanks, that helps. […]
Done
File content/public/test/browser_test_base.h:
Patch Set #9, Line 307:
class RustBrowserTestBase : public Subclass {Oh, thanks-- I was parsing it as "RustBrowserTest Base," but it sounds like it's actually "Rust Brow […]
I think that's a good suggestion, thanks!
File content/public/test/browser_test_macro.rs:
Patch Set #10, Line 1:
// Copyright 2022 The Chromium Authors. All rights reserved.
Is this file part of the "public" interface as well? I don't see extern_browsertest_suite mentioned […]
Yeah, it is used to implement the TestSuite trait for ContentBrowserTest, connecting it to the C++ class and Gtest factory. So it's used from (and to construct) the public API.
```
#[extern_browsertest_suite("content::ContentBrowserTest")]
unsafe impl rust_gtest_interop::TestSuite for ContentBrowserTest {}
```
File content/public/test/content_browser_test.rs:
Patch Set #9, Line 1:
// Copyright 2022 The Chromium Authors. All rights reserved.
Ah, I clearly missed the key line of this file, since I thought smoke_rust_browsertest. […]
The dependency in the GN file mean it depends on the content_public_browsertest_support crate, and the content_public_browsertest_support.rs file is the root of that crate. This file can use anything from any of the things it depends on, which is what it's doing here.
File content/public/test/content_browser_test.rs:
Patch Set #10, Line 19:
inherit usenit: Drop one of these words?
Done
File content/public/test/content_public_browsertest_support.rs:
Patch Set #9, Line 1:
// Copyright 2022 The Chromium Authors. All rights reserved.
Thanks. […]
Sure, it's going to get really redundant fast, as every GN target will have a crate root. I can see how it's helpful for now.
File content/public/test/content_public_test_support.rs:
Patch Set #9, Line 1:
// Copyright 2022 The Chromium Authors. All rights reserved.
As with the other cases, I'm mainly trying to draw a line between "what do people writing tests outs […]
Ah, ok. content_public_browsertest_support depends on content_public_test_support. The former uses the macro defined in the latter to implement the `rust_gtest_interop::TestSuite` trait for `content_public_browsertest_support::ContentBrowserTest`. For this CL, the macro could be made private, as it's only needed for implementing ContentBrowserTest.
However for //chrome browser tests, they will need to wrap InProcessBrowserTest in Rust, and will need to make use of that same macro.
So you don't need public access to the macro to write content browser tests. But you do to use a different subclass of BrowserTestBase. That's why I chose to make it public even though it's not yet needed. WDYT?
To view, visit change 3645146. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Attention is currently required from: Charlie Reis, danakj.
Patch set 12:Code-Review +1
1 comment:
File content/browser/smoke_rust_browsertest.rs:
Not for this CL / brainstorming mode: I wonder if having a single attribute might help alleviate some of the earlier questions and concern (by having a *single* attribute name that needs to be looked up in docs + having more descriptive property names):
#[gtest(
test_suite = "SmokeRustBrowserTest",
test_name = "Smoke",
test_base_class = content_public_browsertest_support::ContentBrowserTest)]
To view, visit change 3645146. To unsubscribe, or for help writing mail filters, visit settings.
Charlie Reis (Gerrit)
Attention is currently required from: danakj.
Patch set 12:Code-Review +1
5 comments:
Commit Message:
Patch Set #10, Line 15:
we introduce some way to restrict that.No, I didn't need any private content stuff in the test. […]
Thanks, that helps.
Patchset:
Thanks, LGTM with one final question for you.
File content/public/test/browser_test_macro.rs:
Patch Set #10, Line 1:
// Copyright 2022 The Chromium Authors. All rights reserved.
Yeah, it is used to implement the TestSuite trait for ContentBrowserTest, connecting it to the C++ c […]
Ack
File content/public/test/content_browser_test.rs:
Patch Set #9, Line 1:
// Copyright 2022 The Chromium Authors. All rights reserved.
The dependency in the GN file mean it depends on the content_public_browsertest_support crate, and t […]
Ack
File content/public/test/content_public_test_support.rs:
Patch Set #9, Line 1:
// Copyright 2022 The Chromium Authors. All rights reserved.
Ah, ok. content_public_browsertest_support depends on content_public_test_support. […]
Ah, that makes more sense. In general, we tend to make things private in content/ until they have an actual use outside of content/, or another CL almost ready to go that will use them outside content/.
In this case, I'm guessing we might have to do some extra work to support making the macro private, if we were to go that route. Which option do you think is more likely for a followup CL-- a chrome/ browser test that needs this to be public, or a content/ CL that needs some kind of non-public APIs? If it's the former, go ahead and leave this public so it can be used (and please introduce a use outside content/ soon if possible). If it's the latter, we might as well show what a private API would look like, and we can switch this to be public later when other browser tests are created.
To view, visit change 3645146. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Patch set 13:Commit-Queue +2
4 comments:
Patchset:
Thanks! I've done a first round and left some questions, largely because this is unfamiliar to me a […]
Done
adetaylor: overall + rustiness […]
Done
File content/browser/smoke_rust_browsertest.rs:
Not for this CL / brainstorming mode: I wonder if having a single attribute might help alleviate som […]
Yeah interesting idea. I was following the model of rstest here fwiw, but we could consider this. I didn't think of it before.
File content/public/test/content_public_test_support.rs:
Patch Set #9, Line 1:
// Copyright 2022 The Chromium Authors. All rights reserved.
Ah, that makes more sense. […]
Ok, I've made it private and added a TODO.
To view, visit change 3645146. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Patch set 13:-Commit-Queue
danakj (Gerrit)
Patch set 14:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
12 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: content/public/test/content_browser_test.rs
Insertions: 1, Deletions: 1.
@@ -3,7 +3,7 @@
// found in the LICENSE file.
use autocxx::prelude::*;
-use content_public_test_support::extern_browsertest_suite;
+use content_private_test_support::extern_browsertest_suite;
include_cpp! {
#include "content/public/test/content_browser_test.h"
```
```
The name of the file: content/test/browser_test_macro.rs
Insertions: 0, Deletions: 0.
The file content/public/test/browser_test_macro.rs was renamed to content/test/browser_test_macro.rs
@@ -2,7 +2,12 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+//! The content-internal test support library.
+
/// The `#[extern_browsertest_suite("cpp::Type")]` macro, which is used to implement the
/// `rust_gtest_interop::TestSuite` trait for Rust wrapper types of C++ subclasses of
/// BrowserTestBase.
+///
+/// TODO(danakj): This will need to move to //content/public/ in order to support subclasses
+/// of BrowserTestBase outside of content, such as InProcessBrowserTest in //chrome.
pub use browser_test_macro::extern_browsertest_suite;
```
```
The name of the file: content/test/content_private_test_support.rs
Insertions: 5, Deletions: 0.
The file content/public/test/content_public_test_support.rs was renamed to content/test/content_private_test_support.rs
@@ -2,7 +2,12 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+//! The content-internal test support library.
+
/// The `#[extern_browsertest_suite("cpp::Type")]` macro, which is used to implement the
/// `rust_gtest_interop::TestSuite` trait for Rust wrapper types of C++ subclasses of
/// BrowserTestBase.
+///
+/// TODO(danakj): This will need to move to //content/public/ in order to support subclasses
+/// of BrowserTestBase outside of content, such as InProcessBrowserTest in //chrome.
pub use browser_test_macro::extern_browsertest_suite;
```
```
The name of the file: content/test/BUILD.gn
Insertions: 22, Deletions: 23.
@@ -32,19 +32,36 @@
}
if (enable_rust) {
- rust_static_library("rust_public_test_support") {
+ rust_static_library("content_private_test_support") {
testonly = true
- crate_name = "content_public_test_support"
- crate_root = "../public/test/content_public_test_support.rs"
+ crate_name = "content_private_test_support"
+ crate_root = "content_private_test_support.rs"
sources = [ crate_root ]
- public_deps = [ ":public_browser_test_macro" ]
+ public_deps = [ ":browser_test_macro" ]
deps = [
":test_support",
"//testing/rust_gtest_interop",
]
+
+ visibility = [ "//content/*" ]
+ }
+
+ rust_macro("browser_test_macro") {
+ testonly = true
+
+ crate_name = "browser_test_macro"
+ crate_root = "browser_test_macro.rs"
+ sources = [ crate_root ]
+ deps = [
+ "//third_party/rust/proc_macro2/v1:lib",
+ "//third_party/rust/quote/v1:lib",
+ ]
+
+ # This target is part of `content_private_test_support`.
+ visibility = [ ":*" ]
}
}
@@ -895,10 +912,9 @@
sources += [ "../public/test/content_browser_test.rs" ]
cxx_bindings = [ "../public/test/content_browser_test.rs" ]
- public_deps = [ ":rust_public_test_support" ]
-
deps = [
":browsertest_support",
+ ":content_private_test_support",
"//testing/rust_gtest_interop",
]
}
@@ -3044,20 +3060,3 @@
]
}
}
-
-if (enable_rust) {
- rust_macro("public_browser_test_macro") {
- testonly = true
-
- crate_name = "browser_test_macro"
- crate_root = "../public/test/browser_test_macro.rs"
- sources = [ crate_root ]
- deps = [
- "//third_party/rust/proc_macro2/v1:lib",
- "//third_party/rust/quote/v1:lib",
- ]
-
- # This target's contents are exposed as part of public test support crates.
- visibility = [ ":*" ]
- }
-}
```
Add the most minimal possible ContentBrowserTest in Rust.
The test is just an empty body so far. But it sets up the instructure
to inherit from content::ContentBrowserTest for the Gtest test suite,
and pass the Rust type for it to the test body.
We shouldn't mix public and private content APIs in a single crate as
that would allow public-API consumers to also use private APIs, unless
we introduce some way to restrict that.
So for now, we have created separate "rust_public_foo" GN targets
for //content/public/test things. This is discussed in detail in
https://crbug.com/1325336, and we will need to revisit how to structure
public vs private things between Rust and C++ based on the outcome
there.
R=adet...@chromium.org, cr...@chromium.org
Bug: 1305396, 1325336
Change-Id: Ic5df4b0ed5313d53e7e441f3800c0d89ee1c5f43
Cq-Include-Trybots: luci.chromium.try:linux-rust-x64-rel,linux-rust-x64-dbg,android-rust-arm-rel,android-rust-arm-dbg
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3645146
Reviewed-by: Adrian Taylor <adet...@chromium.org>
Reviewed-by: Łukasz Anforowicz <luk...@chromium.org>
Reviewed-by: Charlie Reis <cr...@chromium.org>
Commit-Queue: danakj <dan...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1004768}
---
A content/browser/smoke_rust_browsertest.rs
M content/public/test/browser_test_base.h
M content/public/test/content_browser_test.cc
A content/public/test/content_browser_test.rs
A content/public/test/content_public_browsertest_support.rs
M content/test/BUILD.gn
A content/test/browser_test_macro.rs
A content/test/content_private_test_support.rs
8 files changed, 271 insertions(+), 4 deletions(-)