Closed Bug 1457524 Opened 2 years ago Closed 2 years ago

Run unit tests for style, selectors, and servo_arc as part of the rusttests job

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bholley, Assigned: xidorn)

Details

Attachments

(5 files)

Right now we run the stylo unit tests but nothing else, via [1] and [2]. We should fix this to run all the unit tests we want to avoid breaking.

I tried adding style_tests to those places as well, but the build system complained about multiple invocations of RustTest overriding an immutable variable. I don't know enough about the moz.build stuff to fix this, and I'm out of time before heading out on leave. Can somebody take this over?

To do such a build, add --enable-rust-tests to your .mozconfig, and run:

./mach build -C toolkit/library/rust/ check

[1] https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/toolkit/library/rust/Cargo.toml#22
[2] https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/toolkit/library/rust/moz.build#13
Flags: needinfo?(xidorn+moz)
Xidorn may already be aware of this, but the two options to fix the problems bholley was seeing are:

1) Teach the build system to handle multiple RustTest invocations for a given directory.
2) Put every RustTest invocation in its own separate directory.

I *think* that since we are using Rust workspaces, we now have a global Rust target/ directory for the entire build, in which case the two options are more-or-less equivalent.  Option 1 is somewhat more elegant, though.

If we didn't have a global Rust target/ directory, we'd want option 1 to maximize sharing of build artifacts.
Let me experiment a bit.
Assignee: nobody → xidorn+moz
Flags: needinfo?(xidorn+moz)
Note that, the crate hashglobe also contains test, but it cannot be run like other crates because it uses some rustc_private.

We probably need to figure out how to test that, or maybe just remove it somehow? I cannot recall why we need a clone for hash... For fallible allocation?
Hmm... for some reason tests of malloc_size_of_derive don't run well on Linux but works fine on Windows :/
So the message is
> INFO -    Doc-tests malloc_size_of_derive
> INFO -      Running `rustdoc --test /builds/worker/workspace/build/src/servo/components/malloc_size_of_derive/lib.rs ...`
> INFO - error: could not execute process `rustdoc --test /builds/worker/workspace/build/src/servo/components/malloc_size_of_derive/lib.rs ...` (never executed)
> INFO - Caused by:
> INFO -   No such file or directory (os error 2)

I suspect the reason is that the rustc package we use on Linux don't have rustdoc in it.
Actually, every platform has a different message...

Linux 32bit is:
> INFO -      Running `/builds/worker/workspace/build/src/obj-firefox/toolkit/library/release/deps/malloc_size_of_derive-8050f505f536f1b7`
> INFO - /builds/worker/workspace/build/src/obj-firefox/toolkit/library/release/deps/malloc_size_of_derive-8050f505f536f1b7: error while loading shared libraries: libtest-2bb2bef779c43dd6.so: wrong ELF class: ELFCLASS32
> INFO - error: test failed, to rerun pass '--lib'
> INFO - /builds/worker/workspace/build/src/config/rules.mk:967: recipe for target 'force-cargo-test-run' failed

Linux 64bit is comment 7.

Windows 32bit is:
> INFO -      Running `z:/build/build/src/obj-firefox/toolkit/library\release\deps\malloc_size_of_derive-3a2bf398e556c4b8.exe`
> INFO - error: test failed, to rerun pass '--lib'
> INFO - z:/build/build/src/config/rules.mk:967: recipe for target 'force-cargo-test-run' failed
(This does seem pretty similar to Linux 32bit.)
linux32 and win32 smell like bad handling of cross compilation by rust, cargo, or the test itself.
I indeed cannot reproduce the issue in comment 7 locally on a linux vm with rust 1.24.1 installed.
I have another try push with --lib passed to cargo to disable doc-tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e81198bfa912730c6e86ca24bce4208d3b3997f2

In this push, Linux 64bit passes tests of malloc_size_of_derive (but fails on geckodriver somehow doesn't have library target). So it seems the issue on Linux 64bit is indeed related to doc-test.

Linux 32bit and Windows 32bit still fail for the same reason, so it is pretty possible it is a cross compilation issue, although it's weird that it doesn't happen when there is only one crate listed there.
The linux64 doc-test failure is not specific to malloc_size_of_derive (as expected).

linux32 one seems to be specific to that crate, probably because malloc_size_of_derive is a build dependency rather than a normal dependency. I expect win32 to pass this with malloc_size_of_derive removed from the list: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dcb2ecf8a93e67d3bf6c89f7ce7ac7983d93370

linux32 still fails due to some failures in selectors' unit test :O we probably need to investigate why.
So 32bit platforms (linux32 and win32) would fail bloom::create_and_insert_some_stuff in selectors for that there are more false positives than expected (157 is not < 150).

Linux64 is the only one that fails due to not being able to run doc-tests. But it seems that doc-tests are skipped for the two 32bit platforms automatically. So the only platform correctly runs doc-tests is win64.

As I've mentioned in comment 10, I cannot reproduce the linux64 issue on my local vm, so I strongly suspect it is an issue related to the toolchain used by taskcluster.
So, one possible reason of rustdoc issue is that, it is probably a cargo issue that it relies on PATH to contain the rustdoc, rather than invoking the rustdoc with it. The build system seems to invoke cargo and rust with absolute path so that we don't need to put them into PATH. However, invoking of rustdoc is controlled by cargo rather than the build system, and cargo probably decide to invoke rustdoc directly.

Since the Rust toolchain is not in the PATH on the infra, that call would fail for not being able to find the executable.
That problem can be workaround via setting RUSTDOC environment variable, it seems. I guess I can have a patch to fix that, then.

But the bloom filter failure may still need some investigation. Maybe we can just accept that...
Comment on attachment 8972481 [details]
Bug 1457524 part 1 - Use a list for rust tests.

https://reviewboard.mozilla.org/r/241068/#review247436

::: python/mozbuild/mozbuild/frontend/context.py:1253
(Diff revision 1)
>          This variable should not be used directly; you should be using the
>          HostRustLibrary template instead.
>          """),
>  
> -    'RUST_TEST': (unicode, unicode,
> -        """Name of a Rust test to build and run via `cargo test`.
> +    'RUST_TESTS': (List, list,
> +        """Name of a Rust tests to build and run via `cargo test`.

"Names of Rust tests"
Comment on attachment 8972558 [details]
Bug 1457524 part 4 - Allow 16% false positives in test bloom::create_and_insert_some_stuff.

https://reviewboard.mozilla.org/r/241138/#review247438
Attachment #8972558 - Flags: review?(cam) → review+
Comment on attachment 8972482 [details]
Bug 1457524 part 5 - Add crates with tests into the list of RUST_TESTS.

https://reviewboard.mozilla.org/r/241070/#review247440

::: toolkit/library/rust/moz.build:14
(Diff revision 2)
> +    'selectors',
> +    'servo_arc',

Should we add hashglobe too?

::: toolkit/library/rust/moz.build:20
(Diff revision 2)
> +    'servo_arc',
>      'stylo_tests',
>  ]
>  RUST_TEST_FEATURES = gkrust_features
> +
> +if CONFIG['CPU_ARCH'] != 'x86':

Is that the only arch we cross compile?  I wonder if there's a better way to phrase this, like

  if CONFIG['CPU_ARCH'] == config['TARGET_CPU']:
Attachment #8972482 - Flags: review?(cam) → review+
Comment on attachment 8972482 [details]
Bug 1457524 part 5 - Add crates with tests into the list of RUST_TESTS.

https://reviewboard.mozilla.org/r/241070/#review247440

> Should we add hashglobe too?

I tried, but it doesn't work. See comment 5.

> Is that the only arch we cross compile?  I wonder if there's a better way to phrase this, like
> 
>   if CONFIG['CPU_ARCH'] == config['TARGET_CPU']:

We don't run these tests for any other cross compile platforms, I suppose. I don't think we can run, for example, ARM binary on the build machine.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #24)
> I tried, but it doesn't work. See comment 5.

Sorry, I didn't notice that comment.

> We don't run these tests for any other cross compile platforms, I suppose. I
> don't think we can run, for example, ARM binary on the build machine.

OK.  Might be worth mentioning in the comment that this is the only cross-compilation scenario we currently have so we check for that specifically.
You can use `CONFIG['CROSS_COMPILE']` for this purpose, like:
https://dxr.mozilla.org/mozilla-central/rev/ce7072c02389db590c487ca5863b7f00ce22336a/build/unix/elfhack/moz.build#10

If you want tests for cross-compiled builds that's something we can look into. We'd obviously have to build the Rust tests in one job and then run them in a separate job. cargo has a way to do the former via `cargo test --no-run`, but we'd have to do the packaging and running the tests on our own. We have that sort of stuff for CPP_UNIT_TESTS and GTests.
Attachment #8972481 - Flags: review?(core-build-config-reviews) → review?(nfroyd)
Comment on attachment 8972481 [details]
Bug 1457524 part 1 - Use a list for rust tests.

https://reviewboard.mozilla.org/r/241068/#review247556

Thanks for taking this on.

::: python/mozbuild/mozbuild/backend/recursivemake.py:1137
(Diff revision 1)
> -        backend_file.write_once('RUST_TEST := %s\n' % obj.name)
> +        tests = ' '.join('-p ' + name for name in obj.names)
> +        backend_file.write_once('RUST_TESTS := %s\n' % tests)

I have a small preference for making `RUST_TESTS` here the actual list of features, and then having rules.mk insert the necessary `-p` bits, but I'm not sure that it matters that much...

::: python/mozbuild/mozbuild/frontend/context.py:1252
(Diff revision 1)
>  
>          This variable should not be used directly; you should be using the
>          HostRustLibrary template instead.
>          """),
>  
> -    'RUST_TEST': (unicode, unicode,
> +    'RUST_TESTS': (List, list,

As long as we're modifying this, this should be `TypedList(unicode)`, I believe.

::: python/mozbuild/mozbuild/frontend/context.py:1256
(Diff revision 1)
>      'RUST_TEST_FEATURES': (List, list,
> -        """Cargo features to activate for RUST_TEST.
> +        """Cargo features to activate for RUST_TESTS.

I think one of the reasons I didn't enable multiple `RUST_TEST`s is because I wasn't sure how to specify features for each test, if they needed different features.  Thinking about that now, it seems somewhat unlikely that we'd actually need different features for different tests, so I guess this is OK.

::: python/mozbuild/mozbuild/frontend/context.py:1256
(Diff revision 1)
> -
> -        This variable should not be used directly; you should be using the
> -        RustTest template instead.
>          """),
>  
>      'RUST_TEST_FEATURES': (List, list,

Bonus points for making this `TypedList(unicode)` in passing.
Attachment #8972481 - Flags: review?(nfroyd) → review+
Attachment #8972556 - Flags: review?(core-build-config-reviews) → review?(nfroyd)
Comment on attachment 8972556 [details]
Bug 1457524 part 2 - Run all rust tests regardless of failures.

https://reviewboard.mozilla.org/r/241134/#review247558

::: config/rules.mk:966
(Diff revision 1)
>  
>  ifdef RUST_TEST_FEATURES
>  rust_features_flag := --features "$(RUST_TEST_FEATURES)"
>  endif
>  
> +rust_test_flag := --no-fail-fast

Perhaps a comment here on why we want this flag would be appropriate.
Attachment #8972556 - Flags: review?(nfroyd) → review+
Attachment #8972557 - Flags: review?(core-build-config-reviews) → review?(nfroyd)
Comment on attachment 8972557 [details]
Bug 1457524 part 3 - Check rustdoc for rust tests.

https://reviewboard.mozilla.org/r/241136/#review247560
Attachment #8972557 - Flags: review?(nfroyd) → review+
Attachment #8972482 - Flags: review?(core-build-config-reviews) → review?(nfroyd)
Comment on attachment 8972482 [details]
Bug 1457524 part 5 - Add crates with tests into the list of RUST_TESTS.

https://reviewboard.mozilla.org/r/241070/#review247562
Attachment #8972482 - Flags: review?(nfroyd) → review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #26)
> You can use `CONFIG['CROSS_COMPILE']` for this purpose, like:
> https://dxr.mozilla.org/mozilla-central/rev/
> ce7072c02389db590c487ca5863b7f00ce22336a/build/unix/elfhack/moz.build#10

IIRC I actually tried CROSS_COMPILE first, but it doesn't seem to work as expected in terms that the test is still run regardless. I can probably find out the try push later if you are interested.

> If you want tests for cross-compiled builds that's something we can look
> into. We'd obviously have to build the Rust tests in one job and then run
> them in a separate job. cargo has a way to do the former via `cargo test
> --no-run`, but we'd have to do the packaging and running the tests on our
> own. We have that sort of stuff for CPP_UNIT_TESTS and GTests.

Yeah it's probably something we want eventually. We would need two lists of rust tests, one for host, one for target.
Comment on attachment 8972481 [details]
Bug 1457524 part 1 - Use a list for rust tests.

https://reviewboard.mozilla.org/r/241068/#review247556

> I have a small preference for making `RUST_TESTS` here the actual list of features, and then having rules.mk insert the necessary `-p` bits, but I'm not sure that it matters that much...

I think that would be better as well, but I don't know how to do that so I decided to go the easy way first. I can probably try harder to do it in the other way.
Comment on attachment 8972556 [details]
Bug 1457524 part 2 - Run all rust tests regardless of failures.

https://reviewboard.mozilla.org/r/241134/#review247558

> Perhaps a comment here on why we want this flag would be appropriate.

Actually I'm considering removing this now... since it may make it harder to figure out what's going wrong.
Comment on attachment 8972481 [details]
Bug 1457524 part 1 - Use a list for rust tests.

https://reviewboard.mozilla.org/r/241068/#review247556

> I think that would be better as well, but I don't know how to do that so I decided to go the easy way first. I can probably try harder to do it in the other way.

Something like `rust_test_options := $(foreach test,$(RUST_TESTS),-p $(test))` ought to work.
Comment on attachment 8972556 [details]
Bug 1457524 part 2 - Run all rust tests regardless of failures.

https://reviewboard.mozilla.org/r/241134/#review247558

> Actually I'm considering removing this now... since it may make it harder to figure out what's going wrong.

So the reason was that people can see all failures at once, and not need to fix them one by one. But it probably doesn't matter a lot since generally people wouldn't meet more than one failure presumably?
Comment on attachment 8972556 [details]
Bug 1457524 part 2 - Run all rust tests regardless of failures.

https://reviewboard.mozilla.org/r/241134/#review247558

> So the reason was that people can see all failures at once, and not need to fix them one by one. But it probably doesn't matter a lot since generally people wouldn't meet more than one failure presumably?

If there are multiple `sizeof`-style tests in different suites (?), I don't think it'd be unreasonable for a change to affect multiple tests.  And `--no-fail-fast` would be consistent with the way our other test suites are run, I think.
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/619efecc3233
part 1 - Use a list for rust tests. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/e8616e9830d7
part 2 - Run all rust tests regardless of failures. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/4340083e6691
part 3 - Check rustdoc for rust tests. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/904aaf73eb3a
part 4 - Allow 16% false positives in test bloom::create_and_insert_some_stuff. r=heycam
https://hg.mozilla.org/integration/autoland/rev/b4ac7ba11da5
part 5 - Add crates with tests into the list of RUST_TESTS. r=froydnj,heycam
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a5832837830d
followup - Unset RUSTDOC for no-compile environment and add it to mozconfigs.
You need to log in before you can comment on or make changes to this bug.