Specify a Gtest factory by its typename, and give access to a wrapper [chromium/src : main]
danakj (Gerrit)
Attention is currently required from: Łukasz Anforowicz.
1 comment:
Patchset:
A follow-on to https://chromium-review.googlesource.com/c/chromium/src/+/3561744/
It doesn't actually allow anything new :( But it introduces the plumbing/placeholders to do so.
We will need access to the TestSuite to poke at the Browser in a browser test for example. Or the WebContents in a RenderViewHostImplTestHarness test.
What do you think?
To view, visit change 3561744. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Łukasz Anforowicz.
danakj uploaded patch set #2 to this change.
Specify a Gtest factory by its typename, and give access to a wrapper
The wrapper is currently just a useless struct. But in the future can
be a generated wrapper around a real C++ type, and the gtest macros
won't need to invent the type anymore.
R=luk...@chromium.org
Bug: 1305396
Change-Id: I2ddbfbd99d5fb005a22f822a441b9690544dab49
Cq-Include-Trybots: luci.chromium.try:android-rust-arm-dbg,android-rust-arm-rel,linux-rust-x64-dbg,linux-rust-x64-rel
---
M testing/rust_gtest_interop/gtest_attribute.rs
M testing/rust_gtest_interop/rust_gtest_interop.cc
M testing/rust_gtest_interop/rust_gtest_interop.h
M testing/rust_gtest_interop/rust_gtest_interop.rs
M testing/rust_gtest_interop/rust_gtest_interop_unittest.rs
M testing/rust_gtest_interop/test/test_factory.cc
M testing/rust_gtest_interop/test/test_factory.h
7 files changed, 135 insertions(+), 73 deletions(-)
To view, visit change 3561744. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Attention is currently required from: danakj.
7 comments:
File testing/rust_gtest_interop/gtest_attribute.rs:
Just out of curiosity - what is the bug? (I assume that you've filed one)
Patch Set #2, Line 131:
let mut gtest_suite_param = None;One trick that I learned is that you can declare an unbound variable:
let gtest_suite_param: GtestSuiteParam;
and then Rust will verify that the variable is initialized before it is used.It seems that this pattern of calling `syn::Ident::new` gets repeated a few times. I wonder if it can be deduplicated by using either 1) https://docs.rs/quote/1.0.2/quote/macro.format_ident.html or 2) having a helper function similar to `make_rs_ident` from crubit: https://github.com/google/crubit/blob/da9105dadbfac7b6c8e1591b5f4b5e75856fcd95/rs_bindings_from_cc/src_code_gen.rs#L1294-L1305
Patch Set #2, Line 281:
accessed through a reference only.To prevent passing this by value, should this be marked as `!Sized`?
nit: typo?: "given"?
File testing/rust_gtest_interop/rust_gtest_interop.rs:
Patch Set #2, Line 57:
pointer typeIs that comment still accurate? `u8` no longer matches the size of a pointer, right?
Patch Set #2, Line 145:
*mut GtestSuitePtrNot sure if it matters (maybe this is not really public), but the comments elsewhere make me think that GtestSuite is non-trivially-moveable and therefore maybe should be hidden behind `Pin<&mut GtestSuite>`? I dunno... maybe you want to ask Adrian about this.
Edit: I see that you *do* use `Pin` elsewhere. Maybe you didn't use it here, because the impl/body of this function will be FFI/C++ anyway... but... for consistency it should be fine to just use `Pin` and a real Rust reference here (maybe this way avoiding the need for casts elsewhere).
To view, visit change 3561744. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Łukasz Anforowicz.
7 comments:
File testing/rust_gtest_interop/gtest_attribute.rs:
Just out of curiosity - what is the bug? (I assume that you've filed one)
This turned out to be the known #[used] issue: https://github.com/rust-lang/rust/issues/47384
Patch Set #2, Line 131:
let mut gtest_suite_param = None;
One trick that I learned is that you can declare an unbound variable: […]
Hm, in this patchset I test if the param is None later though. I think building on the changes I made in the predecessor, I'll just set a default value here though.
It seems that this pattern of calling `syn::Ident::new` gets repeated a few times. […]
Oh yes. format_ident! is good, thanks.
Patch Set #2, Line 281:
accessed through a reference only.To prevent passing this by value, should this be marked as `!Sized`?
I'm not sure how, without making pointers to the type into wide pointers, do you?
nit: typo?: "given"?
Done
File testing/rust_gtest_interop/rust_gtest_interop.rs:
Patch Set #2, Line 57:
pointer typeIs that comment still accurate? `u8` no longer matches the size of a pointer, right?
Right it's the Test instead of Test*
Patch Set #2, Line 145:
*mut GtestSuitePtr
Not sure if it matters (maybe this is not really public), but the comments elsewhere make me think t […]
Right, so the pin isn't needed until you have a reference. Which is what I did when I casted it and made a reference out of it.
Since this isnt the actual type, I think I would prefer to keep it as a pointer here.
To view, visit change 3561744. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Łukasz Anforowicz.
1 comment:
Patchset:
Ok I did something more here. The macro takes now a C++ type and Rust type.
It has to trust.. that the Rust type is actually an FFI wrapper around the C++ type and they can be casted to each other. I see no way to get typesafety here until we can also generate the factory function. Then the C++ side would fail to compile if you did it wrong, hopefully.
But now we can call methods on the TestSuite.
Any ideas for how to make this better before we have better generators?
To view, visit change 3561744. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Łukasz Anforowicz.
danakj uploaded patch set #5 to this change.
Specify a Gtest factory by its typename, and give access to a wrapper
The gtest macro takes as arguments the C++ typename of the TestSuite to
create (which must have a RUST_GTEST_TEST_SUITE_FACTORY() macro somewhere
that will be linked in with the test) and a Rust type which must be an
FFI wrapper around the C++ type. In particular it must be valid to cast
from a pointer to the C++ type into a pointer to the Rust type.
Since the Rust type is an FFI wrapper and defined by the test file (or
some other crate), it can also expose methods that call through to the
C++ TestSuite class.
R=luk...@chromium.org
Bug: 1305396
Change-Id: I2ddbfbd99d5fb005a22f822a441b9690544dab49
Cq-Include-Trybots: luci.chromium.try:android-rust-arm-dbg,android-rust-arm-rel,linux-rust-x64-dbg,linux-rust-x64-rel
---
M testing/rust_gtest_interop/BUILD.gn
M testing/rust_gtest_interop/gtest_attribute.rs
M testing/rust_gtest_interop/rust_gtest_interop.cc
M testing/rust_gtest_interop/rust_gtest_interop.h
M testing/rust_gtest_interop/rust_gtest_interop.rs
M testing/rust_gtest_interop/rust_gtest_interop_unittest.rs
M testing/rust_gtest_interop/test/test_factory.cc
M testing/rust_gtest_interop/test/test_factory.h
M testing/test.gni
9 files changed, 224 insertions(+), 89 deletions(-)
To view, visit change 3561744. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Łukasz Anforowicz.
danakj uploaded patch set #7 to this change.
Specify a Gtest factory by its typename, and give access to a wrapper
The gtest macro takes as arguments the C++ typename of the TestSuite to
create (which must have a RUST_GTEST_TEST_SUITE_FACTORY() macro
somewhere that will be linked in with the test) and a Rust type which
must be an FFI wrapper around the C++ type. In particular it must be
valid to cast from a pointer to the C++ type into a pointer to the Rust
type.
Since the Rust type is an FFI wrapper and defined by the test file (or
some other crate), it can also expose methods that call through to the
C++ TestSuite class.
R=luk...@chromium.org
Bug: 1305396
Change-Id: I2ddbfbd99d5fb005a22f822a441b9690544dab49
Cq-Include-Trybots: luci.chromium.try:android-rust-arm-dbg,android-rust-arm-rel,linux-rust-x64-dbg,linux-rust-x64-rel
---
M testing/rust_gtest_interop/BUILD.gn
M testing/rust_gtest_interop/gtest_attribute.rs
M testing/rust_gtest_interop/rust_gtest_interop.cc
M testing/rust_gtest_interop/rust_gtest_interop.h
M testing/rust_gtest_interop/rust_gtest_interop.rs
M testing/rust_gtest_interop/rust_gtest_interop_unittest.rs
M testing/rust_gtest_interop/test/test_factory.cc
M testing/rust_gtest_interop/test/test_factory.h
8 files changed, 211 insertions(+), 84 deletions(-)
To view, visit change 3561744. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Attention is currently required from: danakj.
Patch set 13:Code-Review +1
3 comments:
Patchset:
Ok I did something more here. The macro takes now a C++ type and Rust type.
It has to trust.. that the Rust type is actually an FFI wrapper around the C++ type and they can be casted to each other. I see no way to get typesafety here until we can also generate the factory function. Then the C++ side would fail to compile if you did it wrong, hopefully.
But now we can call methods on the TestSuite.
Any ideas for how to make this better before we have better generators?
Brainstorming mode - not sure if the things below are useful.
1) Let's use "Unsafe" in the attribute's name?
2) Or maybe let's introduce an `unsafe` trait that connects `cxx` FFI with the ~manual, RUST_GTEST_TEST_SUITE_FACTORY-based FFI. And then a user would have to define `unsafe impl CantThingOfGoodTraitName for CxxFfiClass` where they would provide the names for the other, ~manual FFI.
But ultimately, fully generated bindings are probably the way to go. BTW: Is it possible to distill the kinds of bindings that you need into a .h header file (I _think_ that it should be possible to express everything in this way, without also requiring C++ bindings for Rust stuff. If so, it would make things easier for `crubit` which only does Rust bindings for C++ stuff for now.). Having a short .h header file would make it easier to use that as one of the goals that `crubit` and `autocxx` should aim for.
Patchset:
Thanks Dana! Sorry for a late reply. It feels a bit rickety, but if it works, then it works and LGTM (as long as we use appropriate disclaimers when presenting or documenting this).
File testing/rust_gtest_interop/rust_gtest_interop.rs:
Patch Set #2, Line 145:
*mut GtestSuitePtrRight, so the pin isn't needed until you have a reference. […]
So I think we are making a choice here between:
1. Having `unsafe` in `test_fn_call` (when casting `suite` to the right type)
2. Having matching types and safe Rust code everywhere, but still having the contract described in the comment you have:// SAFETY: The pointer here comes from C++ which will not move the object inside while
// the test is running. The pointer is only given to Rust once, for the purposes of
// running the test and has a lifetime scoped to the test function.And I think that auto-generated code will do #2, but we want to do #1 for now, while doing the FFI manually. Did I get that right? I guess that works.
To view, visit change 3561744. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
3 comments:
Patchset:
> Ok I did something more here. The macro takes now a C++ type and Rust type. […]
This is brilliant, thank you! I did 2.
The bindings we need is the rust_gtest_interop.h file. If we could use all of it, then we could drop the macro and all the manual C++ mangling I've done.
Patchset:
PTAL
File testing/rust_gtest_interop/rust_gtest_interop.rs:
Patch Set #2, Line 145:
*mut GtestSuitePtr
So I think we are making a choice here between: […]
I have now used Pin<&Mut T> at all the entry/exit points with C++ instead of pointers. I encapsulated the casting in `AsMut<T>` which is implemented for `T: TestSuite`.
To view, visit change 3561744. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
1 comment:
Patchset:
I added a bunch of comments and stuff I think this is good now..
To view, visit change 3561744. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Attention is currently required from: danakj.
Patch set 21:Code-Review +1
1 comment:
Patchset:
Ooops. Should I be taking another look? I am not in the Attention Set so I missed this... :-/
To view, visit change 3561744. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Łukasz Anforowicz.
1 comment:
Patchset:
Ooops. Should I be taking another look? I am not in the Attention Set so I missed this... […]
Oh, yes please do. I changed a lot since you last looked.
I got distracted in holes in the bindings generator and then aliasing stuff, so I forgot about it too.
To view, visit change 3561744. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Attention is currently required from: danakj.
6 comments:
Patchset:
Sending out a handful of comments. Sorry for not finishing the review today. Let me keep myself in the attention set - I'll continue tomorrow/Wednesday.
File testing/rust_gtest_interop/gtest_attribute.rs:
Patch Set #2, Line 281:
accessed through a reference only.I'm not sure how, without making pointers to the type into wide pointers, do you?
Does the following link help:
https://doc.rust-lang.org/beta/unstable-book/language-features/negative-impls.html
?(Not sure if this comment thread is still applicable.)
File testing/rust_gtest_interop/gtest_attribute.rs:
Patch Set #21, Line 116:
default factory methodShould this now say "default test suite"?
let rust_type_name = match parse_gtest_suite(&attr) {
Ok(tokens) => tokens,
Err(error_tokens) => return error_tokens.into(),
};I wonder if this could be written using the `?` syntactic sugar:
let rust_type_name = parse_gtest_suite(&attr)?;
Would that work? (Not sure about the `into()` call in the `Err` branch - maybe this somehow interferes with the `?` syntax.)
nit: Can the name use something test- or test-suite-related? Maybe `GoatTestSuite`? Or maybe `BaseClassForGoatTests`? (When/if renaming please remember to update other references to `Goat` below.)
Patch Set #21, Line 350:
lukaszanit: Can you please update this to `TODO(lukasza, b/229791967)`?
Also - please comment on the bug if needed. I tried guessing what kind of information might be worth preserving, but having more concrete/specific use-cases might be useful (and desirable to capture in the bug).
To view, visit change 3561744. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Attention is currently required from: danakj.
3 comments:
File testing/rust_gtest_interop/README.md:
Patch Set #21, Line 172:
ChosenClassnit: Can the name of the class use something test/gtest-specific? Maybe `CustomTestSuite`? Or maybe `CustomTestSuiteBaseClass`? Or maybe we can use a specific/practical example of `content::BrowserTestBase`?
File testing/rust_gtest_interop/rust_gtest_interop.h:
I would certainly welcome passing non-nullable references instead of pointers. But right now the comment says `Test&` but the code says `Test*` - this probably needs to be reconciled.
File testing/rust_gtest_interop/rust_gtest_interop.rs:
It is not 100% clear to me how one decides how to expose (or where to put) internal details that need to be public:
1. Having them here, but with `#[doc(hidden)]`
VS
2. Having them in the `pub mod __private` module below.
I guess either way works / this is probably not a big deal.
To view, visit change 3561744. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
10 comments:
Patchset:
I finally came back here, yay? Addressed everything - nothing looks controversial so going to submit.
File testing/rust_gtest_interop/README.md:
Patch Set #21, Line 172:
ChosenClass
nit: Can the name of the class use something test/gtest-specific? Maybe `CustomTestSuite`? Or mayb […]
Done
For content::BrowserTestBase that should come with the code to support it, which is another CL.
File testing/rust_gtest_interop/gtest_attribute.rs:
Patch Set #2, Line 281:
accessed through a reference only.
Does the following link help: […]
Right, yeah, but I don't know about using an unstable feature to do this. I'll write a TODO.
File testing/rust_gtest_interop/gtest_attribute.rs:
Patch Set #21, Line 116:
default factory methodShould this now say "default test suite"?
default C++ factory function, which constructs the default test suite, i modified slightly.
let rust_type_name = match parse_gtest_suite(&attr) {
Ok(tokens) => tokens,
Err(error_tokens) => return error_tokens.into(),
};
I wonder if this could be written using the `?` syntactic sugar: […]
? doesn't work cuz it wants to return an Err(TokenStream) but we just need to return TokenStream.
```
error[E0277]: the `?` operator can only be used in a function that returns `Result` or `Option` (or another type that implements `FromResidual`)
```
nit: Can the name use something test- or test-suite-related? Maybe `GoatTestSuite`? Or maybe `BaseC […]
Done
nit: Can the name use something test- or test-suite-related? Maybe `GoatTestSuite`? Or maybe `BaseC […]
Done
Patch Set #21, Line 350:
lukasza
nit: Can you please update this to `TODO(lukasza, b/229791967)`? […]
Done
Patch Set #21, Line 350:
lukasza
nit: Can you please update this to `TODO(lukasza, b/229791967)`? […]
Done
File testing/rust_gtest_interop/rust_gtest_interop.h:
I would certainly welcome passing non-nullable references instead of pointers. […]
Done
To view, visit change 3561744. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Patch set 23:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
21 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: testing/rust_gtest_interop/README.md
Insertions: 21, Deletions: 20.
The diff is too large to show. Please review the diff.
```
```
The name of the file: testing/rust_gtest_interop/rust_gtest_interop.rs
Insertions: 6, Deletions: 3.
The diff is too large to show. Please review the diff.
```
```
The name of the file: testing/rust_gtest_interop/rust_gtest_interop.h
Insertions: 2, Deletions: 2.
The diff is too large to show. Please review the diff.
```
```
The name of the file: testing/rust_gtest_interop/gtest_attribute.rs
Insertions: 10, Deletions: 9.
The diff is too large to show. Please review the diff.
```
Specify a Gtest factory by its typename, and give access to a wrapper
The gtest macro takes as arguments the C++ typename of the TestSuite to
create (which must have a RUST_GTEST_TEST_SUITE_FACTORY() macro
somewhere that will be linked in with the test) and a Rust type which
must be an FFI wrapper around the C++ type. In particular it must be
valid to cast from a pointer to the C++ type into a pointer to the Rust
type.
Since the Rust type is an FFI wrapper and defined by the test file (or
some other crate), it can also expose methods that call through to the
C++ TestSuite class.
R=luk...@chromium.org
Bug: 1305396
Change-Id: I2ddbfbd99d5fb005a22f822a441b9690544dab49
Cq-Include-Trybots: luci.chromium.try:android-rust-arm-dbg,android-rust-arm-rel,linux-rust-x64-dbg,linux-rust-x64-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3561744
Commit-Queue: danakj <dan...@chromium.org>
Reviewed-by: Łukasz Anforowicz <luk...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1002837}
---
M testing/rust_gtest_interop/BUILD.gn
M testing/rust_gtest_interop/README.md
M testing/rust_gtest_interop/gtest_attribute.rs
M testing/rust_gtest_interop/rust_gtest_interop.cc
M testing/rust_gtest_interop/rust_gtest_interop.h
M testing/rust_gtest_interop/rust_gtest_interop.rs
M testing/rust_gtest_interop/rust_gtest_interop_unittest.rs
D testing/rust_gtest_interop/test/test_factory.h
R testing/rust_gtest_interop/test/test_subclass.cc
A testing/rust_gtest_interop/test/test_subclass.h
A testing/rust_gtest_interop/test/test_subclass.rs
11 files changed, 447 insertions(+), 150 deletions(-)