Move tests for the base crate into base_unittests [chromium/src : main]
danakj (Gerrit)
Attention is currently required from: Adrian Taylor.
To view, visit change 3509990. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Adrian Taylor.
1 comment:
File base/values_deserialization.rs:
Patch Set #4, Line 232:
fn test_create() {I didn't move this test, as it's just testing creation of this non-public type, and the json test already uses this code path.
To view, visit change 3509990. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Adrian Taylor.
danakj uploaded patch set #5 to this change.
Move tests for the base crate into base_unittests
This makes use of the #[gtest] macro to define Rust tests inside the
base_unittests binary. The tests use the base crate's public API (which
includes a ForTesting function).
This will enable the tests on the Rust FYI bots, as well as
demonstrate a usage of cross-component Rust code in a component build.
As there are now 2 crate roots in //base, one for prod and one for unit
tests, we rename base/lib.rs to base/base.rs in order to make it more
clear.
After this change, base_unittests includes:
Note: Google Test filter = Rust*
[==========] Running 7 tests from 2 test suites.
[----------] Global test environment set-up.
[----------] 6 tests from RustValuesTest
[ RUN ] RustValuesTest.SetDict
[ OK ] RustValuesTest.SetDict (0 ms)
[ RUN ] RustValuesTest.SetList
[ OK ] RustValuesTest.SetList (0 ms)
[ RUN ] RustValuesTest.ReuseSlot
[ OK ] RustValuesTest.ReuseSlot (0 ms)
[ RUN ] RustValuesTest.StartsNone
[ OK ] RustValuesTest.StartsNone (0 ms)
[ RUN ] RustValuesTest.SetSimpleOptionalValues
[ OK ] RustValuesTest.SetSimpleOptionalValues (0 ms)
[ RUN ] RustValuesTest.AllocDealloc
[ OK ] RustValuesTest.AllocDealloc (0 ms)
[----------] 6 tests from RustValuesTest (0 ms total)
[----------] 1 test from RustJsonParserTest
[ RUN ] RustJsonParserTest.DecodeJson
[ OK ] RustJsonParserTest.DecodeJson (0 ms)
[----------] 1 test from RustJsonParserTest (0 ms total)
[----------] Global test environment tear-down
[==========] 7 tests from 2 test suites ran. (0 ms total)
[ PASSED ] 7 tests.
R=adet...@chromium.org
Bug: 1293979, 1296156
Change-Id: I3c0b9c7724fac72e33aba635c4a7e96d9a76b0d4
Cq-Include-Trybots: luci.chromium.try:android-rust-arm-rel,linux-rust-x64-rel
---
M base/BUILD.gn
R base/base.rs
A base/base_unittests.rs
M base/json/json_parser.rs
A base/json/json_parser_unittest.rs
M base/values.rs
M base/values_deserialization.rs
A base/values_unittest.rs
M build/rust/mixed_target.gni
M build/rust/tests/test_mixed_component/BUILD.gn
M build/rust/tests/test_mixed_executable/BUILD.gn
M build/rust/tests/test_mixed_shared_library/BUILD.gn
M build/rust/tests/test_mixed_static_library/BUILD.gn
M build/rust/tests/test_mixed_testonly_executable/BUILD.gn
M build/rust/tests/test_variable_static_library/BUILD.gn
15 files changed, 188 insertions(+), 136 deletions(-)
To view, visit change 3509990. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Adrian Taylor.
danakj uploaded patch set #6 to this change.
Move tests for the base crate into base_unittests
This makes use of the #[gtest] macro to define Rust tests inside the
base_unittests binary. The tests use the base crate's public API (which
includes a ForTesting function).
This will enable the tests on the Rust FYI bots, as well as
demonstrate a usage of cross-component Rust code in a component build.
As there are now 2 crate roots in //base, one for prod and one for unit
tests, we rename base/lib.rs to base/base.rs in order to make it more
clear.
After this change, base_unittests includes (when rust is enabled):
danakj (Gerrit)
Attention is currently required from: Adrian Taylor.
danakj uploaded patch set #10 to this change.
Move tests for the base crate into base_unittests
This makes use of the #[gtest] macro to define Rust tests inside the
base_unittests binary. The tests use the base crate's public API (which
includes a ForTesting function).
This will enable the tests on the Rust FYI bots, as well as
demonstrate a usage of cross-component Rust code in a component build.
As there are now 2 crate roots in //base, one for prod and one for unit
tests, we rename base/lib.rs to base/base.rs in order to make it more
clear.
We make the base and base_unittests crate roots always exist (when
enable_rust=true), and then conditionally enable the JSON parser and
Values support when the build_rust_json_parser buildflag is enabled,
with some TODOs around proper buildflag support. The Value support
should move out of the JSON parser's buildflag later.
A base/base.rs
A base/base_unittests.rs
M base/json/json_parser.rs
A base/json/json_parser_unittest.rs
D base/lib.rs
M base/values.rs
M base/values_deserialization.rs
A base/values_unittest.rs
M build/rust/mixed_target.gni
M build/rust/tests/test_mixed_component/BUILD.gn
M build/rust/tests/test_mixed_executable/BUILD.gn
M build/rust/tests/test_mixed_shared_library/BUILD.gn
M build/rust/tests/test_mixed_static_library/BUILD.gn
M build/rust/tests/test_mixed_testonly_executable/BUILD.gn
M build/rust/tests/test_variable_static_library/BUILD.gn
16 files changed, 242 insertions(+), 150 deletions(-)
To view, visit change 3509990. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Adrian Taylor.
danakj uploaded patch set #13 to this change.
Move tests for the base crate into base_unittests
This makes use of the #[gtest] macro to define Rust tests inside the
base_unittests binary. The tests use the base crate's public API (which
includes a ForTesting function).
This will enable the tests on the Rust FYI bots, as well as
demonstrate a usage of cross-component Rust code in a component build.
As there are now 2 crate roots in //base, one for prod and one for unit
tests, we rename base/lib.rs to base/base.rs in order to make it more
clear.
We make the base and base_unittests crate roots always exist (when
enable_rust=true), and then conditionally enable the JSON parser and
Values support when the build_rust_json_parser buildflag is enabled,
with some TODOs around proper buildflag support. The Value support
should move out of the JSON parser's buildflag later.
And we avoid building the Rust part of base_unittests on IOS since
the IOS test() templates do not support mixing in Rust with C++ yet.
Bug: 1293979, 1296156, 1304253
Change-Id: I3c0b9c7724fac72e33aba635c4a7e96d9a76b0d4
Cq-Include-Trybots: luci.chromium.try:android-rust-arm-rel,linux-rust-x64-rel
---
M base/BUILD.gn
A base/base.rs
A base/base_unittests.rs
M base/json/json_parser.rs
A base/json/json_parser_unittest.rs
D base/lib.rs
M base/values.rs
M base/values_deserialization.rs
A base/values_unittest.rs
M build/rust/mixed_target.gni
M build/rust/tests/test_mixed_component/BUILD.gn
M build/rust/tests/test_mixed_executable/BUILD.gn
M build/rust/tests/test_mixed_shared_library/BUILD.gn
M build/rust/tests/test_mixed_static_library/BUILD.gn
M build/rust/tests/test_mixed_testonly_executable/BUILD.gn
M build/rust/tests/test_variable_static_library/BUILD.gn
16 files changed, 257 insertions(+), 155 deletions(-)
To view, visit change 3509990. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Adrian Taylor.
danakj uploaded patch set #15 to this change.
Move tests for the base crate into base_unittests
This makes use of the #[gtest] macro to define Rust tests inside the
base_unittests binary. The tests use the base crate's public API (which
includes a ForTesting function).
This will enable the tests on the Rust FYI bots, as well as
demonstrate a usage of cross-component Rust code in a component build.
As there are now 2 crate roots in //base, one for prod and one for unit
tests, we rename base/lib.rs to base/base.rs in order to make it more
clear.
Any assert_eq!() macros are converted to the gtest-friendly expect_eq!()
in order to not take down the whole test suite on a failure.
16 files changed, 272 insertions(+), 167 deletions(-)
To view, visit change 3509990. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Attention is currently required from: danakj, Adrian Taylor.
7 comments:
File base/BUILD.gn:
Should that be inside `if (enable_rust) {` (to avoid accidentally affecting Chromium builds when Rust is not enabled)?
Patch Set #17, Line 3409:
!is_iosCan this say `if (enable_rust)` instead?
Patch Set #17, Line 3449:
is_iosSame question: Can we say `if (enable_rust)` instead?
File base/base_unittests.rs:
Not for this CL, but I can see how this is a strong argument against duplicating this information in `rs_sources`... Maybe the crate roots (like this file) can be auto-generated? WDYT?
File base/lib.rs:
I guess this ends up being marked as deletion even though you did `git mv`? :-/
File base/values_unittest.rs:
nitty nit: I guess `rustfmt` ignores macros (like `expect_eq!`) but maybe this can be manually broken into multiple lines?
"first line\
second line\
third line"
?
File build/rust/mixed_target.gni:
Patch Set #17, Line 140:
skip_unit_tests = trueAFAIU this changes the default, because 95% of cases (all Chromium first-party code?) people will want GTest-based Rust tests and only in the remaining cases (third-party crates?) will native-Rust-unit-tests be used. Did I get that right?
Would it be desirable to rename this variable (and more importantly, the corresponding `rs_...` flag) to `build_native_rust_unit_tests` and make it default to `false`? WDYT?
To view, visit change 3509990. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Adrian Taylor, Łukasz Anforowicz.
8 comments:
Patchset:
No code changes yet, need to debug nacl still
File base/BUILD.gn:
Should that be inside `if (enable_rust) {` (to avoid accidentally affecting Chromium builds when Rus […]
mixed_target intentionally does nothing with rs_* when enable_rust is false
Patch Set #17, Line 3409:
!is_iosCan this say `if (enable_rust)` instead?
it is intended to not be a branch at all eventually. mixed_target takes care of it already (except nacl problems)
but test() for ios is missing support for mixed targets.
Patch Set #17, Line 3449:
is_iosSame question: Can we say `if (enable_rust)` instead?
Same answer :)
File base/base_unittests.rs:
Not for this CL, but I can see how this is a strong argument against duplicating this information in […]
I want to autogenerate test crate roots. But the question of "do we need to list all the sources" made me wonder. In either direction I dont want to duplicate (for tests)
File base/lib.rs:
I guess this ends up being marked as deletion even though you did `git mv`? :-/
Yeah, it changed too much for gerrit.
File base/values_unittest.rs:
nitty nit: I guess `rustfmt` ignores macros (like `expect_eq!`) but maybe this can be manually broke […]
good idea
File build/rust/mixed_target.gni:
Patch Set #17, Line 140:
skip_unit_tests = true
AFAIU this changes the default, because 95% of cases (all Chromium first-party code?) people will wa […]
Yeah good idea! I will rename in another CL
To view, visit change 3509990. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Attention is currently required from: danakj, Adrian Taylor.
Patch set 17:Code-Review +1
4 comments:
Patchset:
LGTM after the remaining small changes are addressed.
File base/BUILD.gn:
mixed_target intentionally does nothing with rs_* when enable_rust is false
Oh, ok. Thanks for explaining this. This (this = hiding handling of enable_rust / ignoring rs_... stuff inside of Rust-specific GN templates) seems like a desirable direction. I hope that the ios/test/mixed trouble is a one-off trouble... (?).
Patch Set #17, Line 3409:
!is_ios
it is intended to not be a branch at all eventually. […]
Ack
Patch Set #17, Line 3449:
is_ios
Same answer :)
Ack
To view, visit change 3509990. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Adrian Taylor.
Patch set 18:Commit-Queue +2
1 comment:
File base/BUILD.gn:
Patch Set #18, Line 103:
toolchain_has_rustI restored the use of toolchain_has_rust here. And I filed a bug about clang_x86 toolchain. C++ does need to know if it should call the Rust backend or not.
However this isn't really JSON specific so much, I think we could make a global HAS_RUST build flag of some sort. That would let us remove checking the flag in .rs files.
To view, visit change 3509990. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Adrian Taylor.
Patch set 18:-Commit-Queue
danakj (Gerrit)
Attention is currently required from: Adrian Taylor.
Patch set 19:Commit-Queue +2
danakj (Gerrit)
Attention is currently required from: Adrian Taylor.
2 comments:
File base/values_unittest.rs:
good idea
Done
File build/rust/mixed_target.gni:
Patch Set #17, Line 140:
skip_unit_tests = true
Yeah good idea! I will rename in another CL
Ack
To view, visit change 3509990. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
17 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: base/BUILD.gn
Insertions: 3, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: build/rust/mixed_target.gni
Insertions: 3, Deletions: 10.
The diff is too large to show. Please review the diff.
```
```
The name of the file: base/values_unittest.rs
Insertions: 15, Deletions: 2.
The diff is too large to show. Please review the diff.
```
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3509990
Reviewed-by: Łukasz Anforowicz <luk...@chromium.org>
Commit-Queue: danakj <dan...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#979275}
---
M base/BUILD.gn
A base/base.rs
A base/base_unittests.rs
M base/json/json_parser.rs
A base/json/json_parser_unittest.rs
D base/lib.rs
M base/values.rs
M base/values_deserialization.rs
A base/values_unittest.rs
M build/rust/mixed_target.gni
M build/rust/std/BUILD.gn
M build/rust/tests/test_mixed_component/BUILD.gn
M build/rust/tests/test_mixed_executable/BUILD.gn
M build/rust/tests/test_mixed_shared_library/BUILD.gn
M build/rust/tests/test_mixed_static_library/BUILD.gn
M build/rust/tests/test_mixed_testonly_executable/BUILD.gn
M build/rust/tests/test_variable_static_library/BUILD.gn
17 files changed, 343 insertions(+), 218 deletions(-)