Allow Rust to provide a different testing::Test subclass for a Gtest. [chromium/src : main]

danakj (Gerrit)

unread,

Mar 10, 2022, 6:06:52 PM3/10/22

to rust...@chromium.org, Chromium LUCI CQ, Łukasz Anforowicz, chromium...@chromium.org, Matthew Riley

Attention is currently required from: Łukasz Anforowicz.

View Change

To view, visit change 3518267. To unsubscribe, or for help writing mail filters, visit settings.

Łukasz Anforowicz (Gerrit)

unread,

Mar 10, 2022, 7:46:22 PM3/10/22

to danakj, rust...@chromium.org, Chromium LUCI CQ, chromium...@chromium.org, Matthew Riley

Attention is currently required from: danakj.

View Change

6 comments:

File testing/rust_gtest_interop/rust_gtest_interop.h:
- Patch Set #4, Line 21: *
  Can this be a reference?
- Patch Set #4, Line 21: *
  Can this also be a reference?
- Patch Set #4, Line 27: unique_ptr
  I don't see the `unique_ptr` that is mentioned in the comment?
- Patch Set #4, Line 42: *
  Can this be a reference? (no difference on the other side of the FFI, so it should be okay I think)
File testing/rust_gtest_interop/rust_gtest_interop.rs:
- Patch Set #4, Line 88: GTestPlaceholder
  Would it make sense to rename this to `TestingTestPlaceholder`? This is supposed to be an opaque equivelent of `testing::Test`, right? Or did I misunderstand this?
- Patch Set #4, Line 107: *const extern "C" fn(GTestPlaceholder)
  I think that:
  1. `*const extern "C" fn(GTestPlaceholder)` is a pointer.
  2. C++ GtestFactory is a pointer
  And therefore everything works across the FFI boundary.
  But... I think `*const extern "C" fn(GTestPlaceholder)` doesn't match the type of the C++ pointer returned from the `rust_gtest_default_factory` function. I think you want to say:
```
    #[repr(C)] struct TestingTestPlaceholder(i32);
    unsafe fn rust_gtest_default_factory() -> extern "C" fn() -> *mut TestingTestPlaceholder {
        ...
    }
    
or just declare an opaque pointer:
```
```
    #[repr(C)] struct GTestFactoryPtr(usize)
    unsafe fn rust_gtest_default_factory() -> GTestFactoryPtr {
        ...
    }
```
  I hope that makes sense / that I didn't misunderstand this?

To view, visit change 3518267. To unsubscribe, or for help writing mail filters, visit settings.

danakj (Gerrit)

unread,

Mar 15, 2022, 9:18:12 AM3/15/22

to rust...@chromium.org, Chromium LUCI CQ, Łukasz Anforowicz, chromium...@chromium.org, Matthew Riley

Attention is currently required from: Łukasz Anforowicz.

View Change

7 comments:

Patchset:
- Patch Set #4:
  PTAL
File testing/rust_gtest_interop/rust_gtest_interop.h:

- Patch Set #4, Line 21: *
  Can this be a reference?

- I don't think that an FFI function can be a reference. CXX does it by writing a pointer version somewhere else that calls with the referece.

- Patch Set #4, Line 21: *
  Can this also be a reference?

- Same

- Patch Set #4, Line 27: unique_ptr
  I don't see the `unique_ptr` that is mentioned in the comment?

- Oh I removed it, fixing

- Patch Set #4, Line 42: *
  Can this be a reference? (no difference on the other side of the FFI, so it should be okay I think)

- https://stackoverflow.com/questions/1906000/c-by-reference-argument-and-c-linkage says we should not use references.
File testing/rust_gtest_interop/rust_gtest_interop.rs:

- Patch Set #4, Line 88: GTestPlaceholder

- Would it make sense to rename this to `TestingTestPlaceholder`? This is supposed to be an opaque eq […]
  I liked your GTestFactoryPtr suggest below, used that.

- Patch Set #4, Line 107: *const extern "C" fn(GTestPlaceholder)

- I think that: […]
  I like the GTestFactoryPtr suggestion.

To view, visit change 3518267. To unsubscribe, or for help writing mail filters, visit settings.

danakj (Gerrit)

unread,

Mar 15, 2022, 1:01:47 PM3/15/22

to rust...@chromium.org, Chromium LUCI CQ, Łukasz Anforowicz, chromium...@chromium.org, Matthew Riley

Attention is currently required from: Łukasz Anforowicz.

View Change

1 comment:

File testing/rust_gtest_interop/rust_gtest_interop.rs:
- Patch Set #6, Line 107: unsafe fn rust_gtest_default_factory() -> extern "C" fn() -> GTestFactoryPtr {
  Now property returning a function pointer here! Thanks!

To view, visit change 3518267. To unsubscribe, or for help writing mail filters, visit settings.

danakj (Gerrit)

unread,

Mar 15, 2022, 1:16:18 PM3/15/22

to rust...@chromium.org

Attention is currently required from: Łukasz Anforowicz.

danakj uploaded patch set #7 to this change.

View Change

Allow Rust to provide a different testing::Test subclass for a Gtest.

Rust will still always provide the default for now. In the future it
will call something other than rust_gtest_default_factory() in order
to get a function pointer that makes a subclass of BrowserTestBase.

By putting the class creation behind a function pointer abstraction,
we can generate the factory from any module. And in particular, from
in //content where BrowserTestBase is known.

There's no functional change in this CL, but the tests continue to run
with the additional plumbing involved.

R=luk...@chromium.org

Bug: 1305396
Change-Id: I26f7eae8b409622775960a804715e462de0cb738
Cq-Include-Trybots: 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/rust_gtest_interop.cc
M testing/rust_gtest_interop/rust_gtest_interop.h
M testing/rust_gtest_interop/rust_gtest_interop.rs
3 files changed, 132 insertions(+), 35 deletions(-)

To view, visit change 3518267. To unsubscribe, or for help writing mail filters, visit settings.

danakj (Gerrit)

unread,

Mar 15, 2022, 1:17:09 PM3/15/22

to rust...@chromium.org

Attention is currently required from: danakj.

danakj uploaded patch set #8 to this change.

View Change

Allow Rust to provide a different testing::Test subclass for a Gtest.

Rust will still always provide the default for now. In the future it
will call something other than rust_gtest_default_factory() in order
to get a function pointer that makes a subclass of BrowserTestBase.

By putting the class creation behind a function pointer abstraction,
we can generate the factory from any module. And in particular, from
in //content where BrowserTestBase is known.

There's no functional change in this CL, but the tests continue to run
with the additional plumbing involved.

R=luk...@chromium.org

Bug: 1305396
Change-Id: I26f7eae8b409622775960a804715e462de0cb738
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/rust_gtest_interop.cc
M testing/rust_gtest_interop/rust_gtest_interop.h
M testing/rust_gtest_interop/rust_gtest_interop.rs
3 files changed, 132 insertions(+), 35 deletions(-)

To view, visit change 3518267. To unsubscribe, or for help writing mail filters, visit settings.

Łukasz Anforowicz (Gerrit)

unread,

Mar 15, 2022, 1:17:16 PM3/15/22

to danakj, rust...@chromium.org, Chromium LUCI CQ, chromium...@chromium.org, Matthew Riley

Attention is currently required from: danakj.

Patch set 7:Code-Review +1

View Change

To view, visit change 3518267. To unsubscribe, or for help writing mail filters, visit settings.

danakj (Gerrit)

unread,

Mar 15, 2022, 1:17:56 PM3/15/22

to rust...@chromium.org, Łukasz Anforowicz, Chromium LUCI CQ, chromium...@chromium.org, Matthew Riley

Patch set 8:Commit-Queue +2

View Change

1 comment:

Patchset:
- Patch Set #8:
  Thanks for the great reviews Lukasz

To view, visit change 3518267. To unsubscribe, or for help writing mail filters, visit settings.

danakj (Gerrit)

unread,

Mar 15, 2022, 2:20:28 PM3/15/22

to rust...@chromium.org

Attention is currently required from: danakj.

danakj uploaded patch set #9 to this change.

View Change

Allow Rust to provide a different testing::Test subclass for a Gtest.

Rust will still always provide the default for now. In the future it
will call something other than rust_gtest_default_factory() in order
to get a function pointer that makes a subclass of BrowserTestBase.

By putting the class creation behind a function pointer abstraction,
we can generate the factory from any module. And in particular, from
in //content where BrowserTestBase is known.

There's no functional change in this CL, but the tests continue to run
with the additional plumbing involved.

R=luk...@chromium.org

Bug: 1305396
Change-Id: I26f7eae8b409622775960a804715e462de0cb738

Cq-Include-Trybots: android-rust-arm-rel,linux-rust-x64-dbg,linux-rust-x64-rel


---
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
3 files changed, 132 insertions(+), 35 deletions(-)

To view, visit change 3518267. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Mar 15, 2022, 2:20:53 PM3/15/22

to danakj, rust...@chromium.org, Łukasz Anforowicz, chromium...@chromium.org, Matthew Riley

CQ can't continue processing your CL:
 * Failed to parse additional builders: project/bucket and builders must be separated by : in `android-rust-arm-rel,linux-rust-x64-dbg,linux-rust-x64-rel`. Canonical syntax is "Cq-Include-Trybots: project/bucket:builder1,builder2;another/bucket:b3". Multiple lines are allowed.

View Change

To view, visit change 3518267. To unsubscribe, or for help writing mail filters, visit settings.

danakj (Gerrit)

unread,

Mar 15, 2022, 2:20:57 PM3/15/22

to rust...@chromium.org, Łukasz Anforowicz, Chromium LUCI CQ, chromium...@chromium.org, Matthew Riley

Patch set 9:Commit-Queue +2

View Change

To view, visit change 3518267. To unsubscribe, or for help writing mail filters, visit settings.

danakj (Gerrit)

unread,

Mar 15, 2022, 2:35:46 PM3/15/22

to rust...@chromium.org, Łukasz Anforowicz, Chromium LUCI CQ, chromium...@chromium.org, Matthew Riley

Attention is currently required from: danakj.

Patch set 9:Commit-Queue +2

View Change

To view, visit change 3518267. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Mar 15, 2022, 2:35:47 PM3/15/22

to danakj, rust...@chromium.org, Łukasz Anforowicz, chromium...@chromium.org, Matthew Riley

Attention is currently required from: danakj.

CQ can't continue processing your CL:


 * Failed to parse additional builders: project/bucket and builders must be separated by : in `android-rust-arm-rel,linux-rust-x64-dbg,linux-rust-x64-rel`. Canonical syntax is "Cq-Include-Trybots: project/bucket:builder1,builder2;another/bucket:b3". Multiple lines are allowed.

View Change

To view, visit change 3518267. To unsubscribe, or for help writing mail filters, visit settings.

danakj (Gerrit)

unread,

Mar 15, 2022, 2:36:22 PM3/15/22

to rust...@chromium.org

Attention is currently required from: danakj.

danakj uploaded patch set #10 to this change.

View Change

Allow Rust to provide a different testing::Test subclass for a Gtest.

Rust will still always provide the default for now. In the future it
will call something other than rust_gtest_default_factory() in order
to get a function pointer that makes a subclass of BrowserTestBase.

By putting the class creation behind a function pointer abstraction,
we can generate the factory from any module. And in particular, from
in //content where BrowserTestBase is known.

There's no functional change in this CL, but the tests continue to run
with the additional plumbing involved.

R=luk...@chromium.org

Bug: 1305396
Change-Id: I26f7eae8b409622775960a804715e462de0cb738

Cq-Include-Trybots: luci.chromium.try:android-rust-arm-rel,linux-rust-x64-dbg,linux-rust-x64-rel


---
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
3 files changed, 132 insertions(+), 35 deletions(-)

To view, visit change 3518267. To unsubscribe, or for help writing mail filters, visit settings.

danakj (Gerrit)

unread,

Mar 15, 2022, 2:36:36 PM3/15/22

to rust...@chromium.org, Łukasz Anforowicz, Chromium LUCI CQ, chromium...@chromium.org, Matthew Riley

Attention is currently required from: danakj.

Patch set 10:Commit-Queue +2

View Change

To view, visit change 3518267. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Mar 15, 2022, 2:41:00 PM3/15/22

to danakj, rust...@chromium.org, Łukasz Anforowicz, chromium...@chromium.org, Matthew Riley

Chromium LUCI CQ submitted this change.

View Change



7 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.

Approvals:
  Łukasz Anforowicz: Looks good to me
  danakj: Commit

Allow Rust to provide a different testing::Test subclass for a Gtest.

Rust will still always provide the default for now. In the future it
will call something other than rust_gtest_default_factory() in order
to get a function pointer that makes a subclass of BrowserTestBase.

By putting the class creation behind a function pointer abstraction,
we can generate the factory from any module. And in particular, from
in //content where BrowserTestBase is known.

There's no functional change in this CL, but the tests continue to run
with the additional plumbing involved.

R=luk...@chromium.org

Bug: 1305396
Change-Id: I26f7eae8b409622775960a804715e462de0cb738
Cq-Include-Trybots: luci.chromium.try:android-rust-arm-rel,linux-rust-x64-dbg,linux-rust-x64-rel

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3518267
Reviewed-by: Łukasz Anforowicz <luk...@chromium.org>
Commit-Queue: danakj <dan...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#981225}


---
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

3 files changed, 136 insertions(+), 35 deletions(-)

To view, visit change 3518267. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward