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.

Sign up for GitHub

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

Jump to bottom

run coverage before doc-build so it doesn't delete the docs that were just built #1392

Merged
merged 2 commits into from
May 2, 2021
Merged

run coverage before doc-build so it doesn't delete the docs that were just built #1392

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d6098bc
run coverage before doc-build so it doesn't delete the docs that were…
syphar May 2, 2021
c009bcc
test for doc coverage in build test
syphar May 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions src/docbuilder/rustwide_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,16 +516,24 @@ impl RustwideBuilder {
let mut storage = LogStorage::new(LevelFilter::Info);
storage.set_max_size(limits.max_log_size());

// we have to run coverage before the doc-build because currently it
// deletes the doc-target folder.
// https://github.com/rust-lang/cargo/issues/9447
let doc_coverage = match self.get_coverage(target, build, metadata, limits) {
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind adding a build test that coverage is generated? Maybe it could look at the /crate page and see if there's a % sign somewhere (see https://docs.rs/crate/tokio/1.5.0 for an example of what that looks like).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add a test.
Instead of checking the HTML output I would propose we just check if the coverage is in the database, which is good enough to know if the build with coverage worked, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's a better test :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@jyn514 I added the assertion to the build-test

Ok(cov) => cov,
Err(err) => {
log::info!("error when trying to get coverage: {}", err);
log::info!("continuing anyways.");
None
}
};

let successful = logging::capture(&storage, || {
self.prepare_command(build, target, metadata, limits, rustdoc_flags)
.and_then(|command| command.run().map_err(failure::Error::from))
.is_ok()
});
let doc_coverage = if successful {
self.get_coverage(target, build, metadata, limits)?
} else {
None
};

// If we're passed a default_target which requires a cross-compile,
// cargo will put the output in `target/<target>/doc`.
// However, if this is the default build, we don't want it there,
Expand Down Expand Up @@ -720,10 +728,12 @@ mod tests {
"SELECT
r.rustdoc_status,
r.default_target,
r.doc_targets
r.doc_targets,
cov.total_items
FROM
crates as c
INNER JOIN releases AS r ON c.id = r.crate_id
LEFT OUTER JOIN doc_coverage AS cov ON r.id = cov.release_id
WHERE
c.name = $1 AND
r.version = $2",
Expand All @@ -734,6 +744,7 @@ mod tests {

assert_eq!(row.get::<_, bool>("rustdoc_status"), true);
assert_eq!(row.get::<_, String>("default_target"), default_target);
assert!(row.get::<_, Option<i32>>("total_items").is_some());

let mut targets: Vec<String> = row
.get::<_, Value>("doc_targets")
Expand Down