Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: introduce code coverage section #446

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vlrpl
Copy link
Contributor

@vlrpl vlrpl commented Nov 10, 2024

as per title

@atenart atenart added this to the v1.5 milestone Nov 14, 2024
@vlrpl vlrpl added the run-functional-tests Request functional tests to be run by CI label Nov 14, 2024
@vlrpl
Copy link
Contributor Author

vlrpl commented Nov 14, 2024

I snuck the vagrant workaround removal into the series as this should be fixed now and a separate PR was not really needed.
For that reason, the PR is labeled with run-functional-tests.

2.4.3 fixes the dependency issue.
Let's simply use that.

Signed-off-by: Paolo Valerio <[email protected]>
Copy link
Contributor

@atenart atenart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments:

  • I tried using it and it seems cargo-llvm-cov needs to be installed as well as the rustup llvm-tools-preview component.
  • Can you use an example for toolchain so users know what to look for?
  • The CARGO_OPTS use should be in its own paragraph, but not in the default example.
  • Since the html output is optional, maybe let's not use it in the default example but alongside the last paragraph. Eg. "for redability you can ...".

@vlrpl
Copy link
Contributor Author

vlrpl commented Nov 14, 2024

A few comments:

* I tried using it and it seems `cargo-llvm-cov` needs to be installed as well as the rustup `llvm-tools-preview` component.

right, these are needed.

* Can you use an example for `toolchain` so users know what to look for?

this could be whatever it is installed via rustup, so could be any version (maybe names can be assigned).
I guess if we want this, we could use +nightly or +stable as those aliases should be safe enough.

* The `CARGO_OPTS` use should be in its own paragraph, but not in the default example.

* Since the html output is optional, maybe let's not use it in the default example but alongside the last paragraph. Eg. "for redability you can ...".

ok, let's move optional stuff in their paragraph.

@atenart
Copy link
Contributor

atenart commented Nov 14, 2024

* Can you use an example for `toolchain` so users know what to look for?

this could be whatever it is installed via rustup, so could be any version (maybe names can be assigned). I guess if we want this, we could use +nightly or +stable as those aliases should be safe enough.

Could be any version, I think the explanation is good in this regards. I just think using whatever version as an example would gives an idea what is expected. Eg. "with a local tool chain being ... this would look like ...".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-functional-tests Request functional tests to be run by CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants