Allow a #[gtest] test to specify a C++ class for its TestSuite. [chromium/src : main]
danakj (Gerrit)
Attention is currently required from: Łukasz Anforowicz.
Patch set 21:Commit-Queue +1
To view, visit change 3560902. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Łukasz Anforowicz.
danakj has uploaded this change for review.
Allow a #[gtest] test to specify a C++ class for its TestSuite.
By default the testing::Test class is used, which is equivalent to the
C++ TEST() macro. But just as in C++, with the TEST_F() macro, we need
a way to specify a different class to be the TestSuite. The only
requirement is that the test is a subclass of testing::Test.
We generalize the Gtest TestSuite factory code with a templated
function, but Rust can not call templated C++ functions yet. So we
still wrap that function with the pre-existing
rust_gtest_default_factory() function.
This would allow easily writing other factory functions in C++ targets
outside of //testing on top of the template helper, or even generating
such functions in the future.
We augment the #[gtest] macro to look for a further #[gtest_suite]
macro that names a C++ factory for the TestSuite class. The syntax is:
#[gtest_suite(some_factory_function)]
Where the factory function is an unsafe extern "C" function pointer that
receives a function pointer and returns a
rust_gtest_interop::GtestFactoryPtr, making its signature:
unsafe extern "C" fn(f: extern "C" fn()) -> GTestFactoryPtr
In the process, we drop a level of abstraction, so we no longer have a
factory-factory, and Rust works with a function pointer to the C++
function directly.
R=luk...@chromium.org
Bug: 1305396
Change-Id: I1fe5efa8090435133054b3b7e71f28184b9d4733
Cq-Include-Trybots: luci.chromium.try:android-rust-arm-rel,android-rust-arm-dbg,linux-rust-x64-dbg,linux-rust-x64-rel
---
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
M testing/rust_gtest_interop/rust_gtest_interop_unittest_main.cc
A testing/rust_gtest_interop/test/test_factory.cc
A testing/rust_gtest_interop/test/test_factory.h
10 files changed, 308 insertions(+), 79 deletions(-)
Łukasz Anforowicz (Gerrit)
Attention is currently required from: danakj.
4 comments:
File testing/rust_gtest_interop/gtest_attribute.rs:
Patch Set #23, Line 2:
TokenStream2I think that this Stack Overflow answer (and the question above) are quite interesting to read: https://stackoverflow.com/a/63280281/4692014
In particular, it suggests to import items from proc-macro2 (so they can be used unqualified), and just used fully qualified names for proc_macro.
fn #rust_gtest_default_factory(
body: extern "C" fn(),
) -> ::rust_gtest_interop::GtestSuitePtr;
#gtest_factory_fn_signatureOnly one of these is needed at a time, right? Would we be able to simplify the code here a bit, if everything (including the defaults) went through non-optional `gtest_factory_fn` and `gtest_factory_fn_signature`?
File testing/rust_gtest_interop/rust_gtest_interop.h:
The Subclass must be
// `testing::Test`, or a subclass thereofCan this requirement be enforced via `static_assert` / `std::is_base_of`?
File testing/rust_gtest_interop/test/test_factory.h:
// A custom Gtest factory that returns a `TestSubclass`.
extern "C" testing::Test* test_subclass_factory(void (*body)()) {
return rust_gtest_interop::rust_gtest_factory_for_subclass<TestSubclass>(
body);
}
// Returns how many times the test_subclass_factory() function was called.
extern "C" size_t num_subclass_created() {
return TestSubclass::num_created();
}These can be moved into the .cc file, right? I guess having them here is fine, although they aren't *really* part of the public API (they are only used from internals of rust_gtest_interop / are not used by actual test code).
To view, visit change 3560902. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Łukasz Anforowicz.
5 comments:
Patchset:
PTAL
File testing/rust_gtest_interop/gtest_attribute.rs:
Patch Set #23, Line 2:
TokenStream2
I think that this Stack Overflow answer (and the question above) are quite interesting to read: http […]
Oh okay.
fn #rust_gtest_default_factory(
body: extern "C" fn(),
) -> ::rust_gtest_interop::GtestSuitePtr;
#gtest_factory_fn_signature
Only one of these is needed at a time, right? Would we be able to simplify the code here a bit, if […]
Yeah, the next CL was doing something like this as well. Done.
File testing/rust_gtest_interop/rust_gtest_interop.h:
The Subclass must be
// `testing::Test`, or a subclass thereofCan this requirement be enforced via `static_assert` / `std::is_base_of`?
It is enforced when rust_gtest_factory_for_subclass returns it as a Test*. We could also check is_convertable here, though it would be redundant I think, since that's where RustClass is constructed? I suppose someone could (incorrectly) bypass rust_gtest_factory_for_subclass... Done.
File testing/rust_gtest_interop/test/test_factory.h:
// A custom Gtest factory that returns a `TestSubclass`.
extern "C" testing::Test* test_subclass_factory(void (*body)()) {
return rust_gtest_interop::rust_gtest_factory_for_subclass<TestSubclass>(
body);
}
// Returns how many times the test_subclass_factory() function was called.
extern "C" size_t num_subclass_created() {
return TestSubclass::num_created();
}
These can be moved into the . […]
They are used by rust_gtest_interop_unittests only. They and are not part of rust_gtest_interop.
They could go in the .cc yes. I was thinking eventually they'll go away when the unit test can use the TestSubclass, and template functions directly instead. And in the meantime they may get consumed by a binding generator in which case they need to be in the header.
To view, visit change 3560902. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Attention is currently required from: danakj.
Patch set 24:Code-Review +1
3 comments:
Patchset:
Thanks Dana! LGTM.
File testing/rust_gtest_interop/README.md:
Patch Set #24, Line 169:
chosen_class_gtest_factoryMacro expansion will generate a bunch of ~manual FFI declarations. This is okay to get things working. But... I wonder if we should say somewhere here that 1) this is `unsafe` (e.g. if a user provides an incompatible function signature to `gtest_suite` then they get crashes/UB/etc) and 2) that the unsafety can go away after more investment into automated bindings generation (maybe marking this with a TODO(bug) for tracking).
WDYT?
File testing/rust_gtest_interop/gtest_attribute.rs:
Patch Set #23, Line 2:
TokenStream2
Oh okay.
In particular, I hope that this helps with making parts of this unit-testable.
To view, visit change 3560902. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Patch set 24:Commit-Queue +2
1 comment:
File testing/rust_gtest_interop/README.md:
Patch Set #24, Line 169:
chosen_class_gtest_factory
Macro expansion will generate a bunch of ~manual FFI declarations. […]
Let's see how the dust settles on the next CL. I don't think it's good in the way it is now. But with the macro + specifying types it's a bit better, and very different from this.
To view, visit change 3560902. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3560902
Reviewed-by: Łukasz Anforowicz <luk...@chromium.org>
Commit-Queue: danakj <dan...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#987906}
---
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
M testing/rust_gtest_interop/rust_gtest_interop_unittest_main.cc
A testing/rust_gtest_interop/test/test_factory.cc
A testing/rust_gtest_interop/test/test_factory.h
10 files changed, 316 insertions(+), 84 deletions(-)