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

[TK-01182] 2020 Code Coverage #64

Merged
merged 2 commits into from
Apr 9, 2020
Merged

[TK-01182] 2020 Code Coverage #64

merged 2 commits into from
Apr 9, 2020

Conversation

neonphog
Copy link
Contributor

@neonphog neonphog commented Apr 9, 2020

coverage_report

We cannot do codecov.io right now unless we pay for it because we are using a private repo.

This PR just adds a hc-coverage-test command that will pop the report in a browser. We can also make a circle/githubaction job that will block the build if we don't get > some coverage % if we want, but that's not included in this request.

@@ -12,14 +12,15 @@

# can be any github ref
# branch, tag, commit, etc.
ref = "2020-02-27-rust-stable";
ref = "1ca45a6f1899947e7fd0addc951858c110a391a5";
# ^^ this is HEAD of 2020-02-27-rust-stable-2 as of 2020-04-09 (david.b)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't just push to 2020-02-27-rust-stable without breaking everyone who's currently based off develop -- by using the actual commit hash here, this won't be a problem in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean that this hash refers to a commit in holochain/holonix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, exactly

@@ -38,6 +40,7 @@ fn malformed_toml_error_is_friendly() {
}

#[test]
#[ignore] // (david.b) [D-01034] these take minutes to run in nix/CI - disable until fixed
Copy link
Contributor

Choose a reason for hiding this comment

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

Weirdly, I haven't had this problem running the tests outside of nix, but I did see it on Art's nix-shell. What do you make of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't looked into it - usually nix-specific problems have to do with nix's funky linking.

$ readelf -d target/debug/sx_types-66e1c5faaaa26893 | grep RUNPATH
 0x000000000000001d (RUNPATH)            Library runpath: [/nix/store/wac2lyrm69vihamdpx7dnh5n35df2kji-dev-shell/lib64:/nix/store/wac2lyrm69vihamdpx7dnh5n35df2kji-dev-shell/lib:/nix/store/pnd2kl27sag76h23wa5kl95a76n3k9i3-glibc-2.27/lib:/nix/store/4l35nqpaiwzhfafrpby1xf7kfik7ai7c-gcc-8.3.0-lib/lib]

Copy link
Contributor

@timotree3 timotree3 left a comment

Choose a reason for hiding this comment

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

I approve these changes to the code. Nice work!

As we merge this though, I have two follow-up questions that maybe deserve to become new issues:

  • I would like to hear from you @neonphog, what would it take to avoid the need for this hack and instead just have kcov respect CARGO_TARGET_DIR?
  • What would it take to prevent "tooling-rot" by ensuring that this script continues to work as we change things? Do we want to run this in CI? Maybe even as a separate job?

@timotree3 timotree3 merged commit 56b37e5 into develop Apr 9, 2020
@timotree3 timotree3 deleted the code-coverage branch April 9, 2020 17:28
@neonphog
Copy link
Contributor Author

neonphog commented Apr 9, 2020

@timotree3

I would like to hear from you @neonphog, what would it take to avoid the need for this hack and instead just have kcov respect CARGO_TARGET_DIR?

Totally agree - This is a frustration for me as well. But cargo-make seems to limit us to this usage - I keep trying to make it more general, but nothing else works. We may want to bite the bullet and stop using cargo-make in favor of a custom script - or different build coordinator tool, but I don't want to spend too much time researching that.

@neonphog
Copy link
Contributor Author

neonphog commented Apr 9, 2020

@timotree3

What would it take to prevent "tooling-rot" by ensuring that this script continues to work as we change things? Do we want to run this in CI? Maybe even as a separate job?

Yep, we can totally run this in CI - it'll be a little weird and not accomplish anything since we can't publish the results - I'll bring it up as a question in standup today.

@timotree3
Copy link
Contributor

Yep, we can totally run this in CI - it'll be a little weird and not accomplish anything since we can't publish the results - I'll bring it up as a question in standup today.

Cool. It would feel weird though to delay people getting their CI results as the cost for this. I wonder if ideally we would want a notification that appears on a PR, saying that it broke the tool, but not blocking the PR. Kind of like Rust has for PRs that break clippy or miri

@timotree3

This comment has been minimized.

@timotree3

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants