Add support for cargo test

luca-della-vedova commented 1 month ago

Closes https://github.com/colcon/colcon-cargo/issues/3 This PR improves support for cargo test by adding test result generation, as well as support for doc tests and cargo fmt tests.

I made the following choices:

Collate all results from each command (cargo test and cargo fmt) into a single test and report whether the whole command succeeded or failed as a single test case. This is different from the "grep approach", but that is imho not a very reliable idea because there is no guarantee that the human readable formatting will stay the same and it might break without any notice. The alternative of using json requires nightly so that is also out of the question (until it is stabilized, at which point we should revisit this implementation).
Export all cargo test results into a single cargo_test.xml file, similar to what we do for Python packages (pytest.xml) but different from what we do for C++ packages. Happy to revisit this if there is a strong feeling.
By default run unit tests and fmt. I haven't implemented command line arguments to set which tests to run yet, that can be done either here or in a followup PR.

What's next:

[ ] Add parameter to enable / disable fmt, unit tests.
[x] Integrate with colcon-ros-cargo. https://github.com/colcon/colcon-ros-cargo/pull/19
[ ] Adding time to the test result file, I didn't add it first because cargo test already reports time but we are not parsing it, and reimplementing a stopwatch in Python feels unnecessary.
[ ] Test granularity (i.e. number of tests, what failed and what didn't). Depend on a larger refactor when json or xml outputs are stable
[ ] Figure out how to add doctests (pure binary packages fail doctest with the same exit code as a failing test)

codecov-commenter commented 1 month ago

Codecov Report

Attention: Patch coverage is 77.55102% with 11 lines in your changes missing coverage. Please review.

Project coverage is 66.02%. Comparing base (9fdb14f) to head (b3ef3cd).

Files	Patch %	Lines
colcon_cargo/task/cargo/test.py	80.00%	4 Missing and 4 partials :warning:
colcon_cargo/task/cargo/build.py	66.66%	1 Missing and 2 partials :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #39 +/- ## =========================================== + Coverage 46.98% 66.02% +19.04% =========================================== Files 6 6 Lines 166 209 +43 Branches 24 30 +6 =========================================== + Hits 78 138 +60 + Misses 74 50 -24 - Partials 14 21 +7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

esteve commented 1 month ago

@luca-della-vedova would it make sense to switch to https://nexte.st/ ? It'd mean depending on the cargo-nextest crate, but it provides output in the JUnit format, which colcon already has parsers for.

luca-della-vedova commented 1 month ago

@luca-della-vedova would it make sense to switch to https://nexte.st/ ? It'd mean depending on the cargo-nextest crate, but it provides output in the JUnit format, which colcon already has parsers for.

I'm mostly neutral about this, while it would make the integration a lot easier and more feature complete (i.e. no need to write our own xml generation, per test output), it has two main drawbacks I can think of:

Require a custom cargo install from people to use colcon-cargo, which right now doesn't need any external crate
Not support doctests for which we would still need to fallback to normal cargo test (as documented here), which would bring us again to "how do we format the output of doctests if we don't have a machine readable output".

Whichever approach we take imho it would just be a stopgap until cargo test can output machine readable format, but that seems to be progressing very slowly

esteve commented 1 month ago

Not support doctests for which we would still need to fallback to normal cargo test (as documented https://github.com/nextest-rs/nextest/issues/16), which would bring us again to "how do we format the output of doctests if we don't have a machine readable output".

I agree, not having support for doctests is unfortunate. If we can support plain cargo test, that'd be great, even if it's just minimal (pass/fail).

luca-della-vedova commented 1 month ago

This should now be ready for review.

I added unit tests to the sample package so the CI should prove that it works but if you want to try yourself:

Install this package (and the matching colcon-ros-cargo if you want to try a ros package)

pip install git+https://github.com/luca-della-vedova/colcon-cargo.git@luca/cargo_test_support --force
pip install git+https://github.com/luca-della-vedova/colcon-ros-cargo.git@luca/add_test --force

Then do a colcon build and colcon test for your package!

I tested it on ros2-rust and rmf_site, for example (current main):

$ colcon test --packages-select rclrs
Starting >>> rclrs   
--- stderr: rclrs                   
Warning: can't set `imports_granularity = Crate`, unstable features are only available in nightly channel.
Warning: can't set `imports_granularity = Crate`, unstable features are only available in nightly channel.
---
Finished <<< rclrs [4.32s]

Summary: 1 package finished [4.59s]
  1 package had stderr output: rclrs

$ cat build/rclrs/cargo_test.xml
<?xml version="1.0" encoding="utf-8"?>
<testsuites>
    <testsuite name="cargo_test" errors="0" failures="0" skipped="0" tests="2">
        <testcase name="unit"/>
        <testcase name="fmt"/>
    </testsuite>
</testsuites>

On the other hand, if I edit the formatting to be a bit messed up and a unit test to fail the output becomes:

$ colcon test --packages-select rclrs
Starting >>> rclrs   
--- stderr: rclrs                   
error: test failed, to rerun pass `--lib`
Warning: can't set `imports_granularity = Crate`, unstable features are only available in nightly channel.
Warning: can't set `imports_granularity = Crate`, unstable features are only available in nightly channel.
---
Finished <<< rclrs [3.94s]  [ with test failures ]

Summary: 1 package finished [4.21s]
  1 package had stderr output: rclrs
  1 package had test failures: rclrs

$ colcon test-result
build/rclrs/cargo_test.xml: 2 tests, 0 errors, 2 failures, 0 skipped

Summary: 13319 tests, 0 errors, 2 failures, 4130 skipped
lucadv@noble:~/ws_rclrs$ cat build/rclrs/cargo_test.xml
<?xml version="1.0" encoding="utf-8"?>
<testsuites>
    <testsuite name="cargo_test" errors="0" failures="2" skipped="0" tests="2">
        <testcase name="unit">
            <failure message="cargo test failed">
running 52 tests
......................F.............................
failures:

---- time::tests::test_conversion stdout ----
thread 'time::tests::test_conversion' panicked at src/time.rs:115:9:
assertion `left == right` failed
  left: 1
 right: 2
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    time::tests::test_conversion

test result: FAILED. 51 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.35s

</failure>
        </testcase>
        <testcase name="fmt">
            <failure message="cargo fmt failed">Diff in /usr/local/google/home/lucadv/ws_rclrs/src/ros2_rust/rclrs/src/time.rs at line 110:
         let msg = time.to_ros_msg().unwrap();
         assert_eq!(msg.nanosec, 100);

-
-
         assert_eq!(msg.sec, 2);
     }
 }
</failure>
        </testcase>
    </testsuite>
</testsuites>

@esteve I can't seem to ask for review from you but happy to have your input

luca-della-vedova commented 1 month ago

I don't think its required to merge this PR, but we really need some documentation in the README for this repo.

It'd be nice to have some kind of translation, this colcon command, translates to this cargo command. Especially now that this isn't a 1:1 (e.x colcon test translates to cargo build ... & cargo fmt ... )

Thanks for all the feedback! For this I added a root level example of colcon test to the README.

Edit: For the translation it's kinda noted under the "What's next" section in Add parameter to enable / disable fmt, unit tests. .

Ideally I would like some command line parameter where users can specify what kind of tests they want, i.e. enable / disable format, doc tests, etc. The risk there however, is the good old "bikeshedding" where I feel we might get stuck on details like what to call the CLI parameter, what it should look like etc. For this reason I thought a first implementation of cargo test and cargo fmt would be good enough. We are lucky in a way that Rust seems to usually have a single linter of choice, compared to C++ / Python that have a lot of them, so I think it's safe to assume that most people will want to run through a simple cargo fmt.

mxgrey commented 2 weeks ago

It seems a pre-existing problem with colcon-cargo is that it will cause colcon build to emit a build error when a library-only package exists in the workspace. I rediscovered this problem while testing this PR.

The good news is that the fix is fairly straightforward, and I've opened a PR to fix this that targets this branch: https://github.com/luca-della-vedova/colcon-cargo/pull/2

luca-della-vedova commented 2 weeks ago

Since https://github.com/luca-della-vedova/colcon-cargo/pull/2 is a bit tangential to this PR that hasn't been widely reviewed yet (it's a new feature to allow building pure library targets, that is not currently supported in colcon-cargo), I'll merge this as is then open a followup for it

colcon / colcon-cargo

Add support for cargo test #39

Codecov Report