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

Interoperability with trybuild #32

Closed
jhpratt opened this issue Jun 15, 2021 · 9 comments · Fixed by #44 or #54
Closed

Interoperability with trybuild #32

jhpratt opened this issue Jun 15, 2021 · 9 comments · Fixed by #44 or #54
Assignees
Labels
C-bug Category: related to a bug. C-enhancement Category: A new feature or an improvement for an existing one

Comments

@jhpratt
Copy link

jhpratt commented Jun 15, 2021

Currently, the trybuild crate does not contribute anything to the coverage report. I'm not sure whether this should be fixed in cargo-llvm-cov or trybuild. compiletest_rs, on the other hand, does contribute to the coverage report (but is far more finicky).

@taiki-e taiki-e added C-bug Category: related to a bug. C-enhancement Category: A new feature or an improvement for an existing one labels Jun 15, 2021
@taiki-e
Copy link
Owner

taiki-e commented Jun 23, 2021

I'm not sure what we can do about this on our side...

  • IIRC, the code that trybuild generates for tests is located in "target/tests".
  • The path to the object file will be collected in the object_files function.
  • The object_files function does not currently search "target/tests/target".
  • However, I tried a change a while ago to search for "target/tests/target", but the coverage results did not change.

cargo-llvm-cov/src/main.rs

Lines 142 to 170 in a04fcab

fn object_files(cx: &Context) -> Result<Vec<OsString>> {
let mut files = vec![];
// To support testing binary crate like tests that use the CARGO_BIN_EXE
// environment variable, pass all compiled executables.
// This is not the ideal way, but the way unstable book says it is cannot support them.
// https://doc.rust-lang.org/nightly/unstable-book/compiler-flags/instrument-coverage.html#tips-for-listing-the-binaries-automatically
let mut build_dir = cx.target_dir.join(if cx.release { "release" } else { "debug" });
if let Some(target) = &cx.target {
build_dir.push(target);
}
fs::remove_dir_all(build_dir.join("incremental"))?;
for f in WalkDir::new(build_dir.as_str()).into_iter().filter_map(Result::ok) {
let f = f.path();
if is_executable::is_executable(&f) {
files.push(f.to_owned().into_os_string());
}
}
if cx.doctests {
for f in glob::glob(cx.doctests_dir.join("*/rust_out").as_str())?.filter_map(Result::ok) {
if is_executable::is_executable(&f) {
files.push(f.into_os_string());
}
}
}
// This sort is necessary to make the result of `llvm-cov show` match between macos and linux.
files.sort_unstable();
trace!(object_files = ?files);
Ok(files)
}

@jhpratt
Copy link
Author

jhpratt commented Jun 23, 2021

Hmm…that's odd. What does compiletest_rs do differently that it is covered?

@jhpratt
Copy link
Author

jhpratt commented Aug 7, 2021

Just coming back to this. I just looked through trybuild's code and see nothing that could conceivably be changed to help this. It (unfortunately) looks like this has to be handled on llvm-cov's end if my understanding is correct. As far as I can tell the bullet points you've listed are correct as they relate to trybuild.

@taiki-e
Copy link
Owner

taiki-e commented Aug 7, 2021

I just found a way to fix this.

@taiki-e taiki-e self-assigned this Aug 7, 2021
@taiki-e taiki-e mentioned this issue Aug 7, 2021
@taiki-e
Copy link
Owner

taiki-e commented Aug 7, 2021

This will be fixed by #44 and dtolnay/trybuild#121.

@bors bors bot closed this as completed in b42adf7 Aug 7, 2021
@jhpratt
Copy link
Author

jhpratt commented Aug 7, 2021

You're a wizard, what can I say. You always find a way to do things, and generally quite quickly at that.

@davidhewitt
Copy link
Contributor

🚀 thank you, really excited to see this!

@taiki-e
Copy link
Owner

taiki-e commented Aug 10, 2021

trybuild 1.0.44 yanked due to regression: dtolnay/trybuild#122

@taiki-e
Copy link
Owner

taiki-e commented Aug 11, 2021

FYI: 1.0.34 or older versions of trybuild provide similar behavior to 1.0.44, so you can work around this issue by pinning trybuild to those versions for now.

trybuild = "=1.0.34"

bors bot added a commit that referenced this issue Aug 15, 2021
54: Support trybuild (take 2) r=taiki-e a=taiki-e

Fixes #32

The following patch is needed until dtolnay/trybuild#123 is merged:

```toml
[patch.crates-io]
trybuild = { git = "https://github.com/taiki-e/trybuild.git", branch = "target" }
```

---

Tested in the same way as #44 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
@bors bors bot closed this as completed in 36972a1 Aug 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: related to a bug. C-enhancement Category: A new feature or an improvement for an existing one
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants