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

tarpaulin broken #676

Closed
michaelkirk opened this issue Nov 2, 2021 · 4 comments
Closed

tarpaulin broken #676

michaelkirk opened this issue Nov 2, 2021 · 4 comments

Comments

@michaelkirk
Copy link
Member

Tarpaulin is the tool we use in CI to measure how much of our code is exercised by tests. If we have less coverage, it's supposed to fail.

As of recently, tarpaulin itself is failing to run: https://github.com/georust/geo/runs/4081475865?check_suite_focus=true

A quick search of the error message lead me here: xd009642/tarpaulin#756

I'm not sure if that's the issue we're hitting, but... my understanding of that issue is is some dependency crates, in particular some older ones, aren't compatible with tarpaulin's instrumentation.

It's possible that there are some deps that we could update and maybe that'd fix it?

...or I'd personally be happy to remove tarpaulin. I understand the philosophy of more code coverage == more better, but in practice tarpaulin has been a slight drag and given spurious results. And I don't know that it's ever gotten me to write more tests. Like it'll fail, then succeed when run again.

Maybe it just needs more tender configuration, but I'm not personally interested in spending my time on that.

@urschrei
Copy link
Member

urschrei commented Nov 2, 2021

I haven't seen it flapping in geo, but I agree that it's been historically temperamental. We "culturally enforce" (or just "expect") extensive testing in geo and we've never actually gated merges on decreased coverage, so in the absence of another simple solution (what happens if we remove the coverage job from ci-result.needs ?) I'm fine with getting rid of a moving part that isn't reliable from our testing infra.

urschrei added a commit to urschrei/geo that referenced this issue Nov 3, 2021
bors bot added a commit that referenced this issue Nov 4, 2021
677: Remove cargo-tarpaulin due to instability r=urschrei a=urschrei

See discussion at #676

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---



Co-authored-by: Stephan Hügel <shugel@tcd.ie>
@lnicola
Copy link
Member

lnicola commented Nov 4, 2021

Presumably closed by #677.

@lnicola lnicola closed this as completed Nov 4, 2021
@xd009642
Copy link

xd009642 commented Nov 5, 2021

Just an FYI the next version of bindgen will remove the troublesome version of bitvec from the dependency tree, or the --avoid-cfg-tarpaulin flag would have fixed the CI issue.

As for that issue, bitvec and time 0.2 were the only crates using that deprecated tarpaulin feature and when I did a check of public tarpaulin users using the feature they didn't show up in the search results so it seemed a safe breaking change. I've personally not seen tarpaulin "flapping" irl except for an instance where it was unveiling an issue in rustc so any projects that demonstrate that behaviour would be great to forward to me. I'd understand if you don't use tarpaulin if it's unstable but I'd still like to know the weaknesses so I can work on them 🤷

@michaelkirk
Copy link
Member Author

Hi @xd009642 - thanks for following our issue and I'm sorry for not have more actionable constructive feedback.

I've personally not seen tarpaulin "flapping" irl

My memory is a little bit fuzzy on this, but I think what I'm actually recalling was not an issue with tarpaulin, but with coveralls - the service which was tracking our code coverage over time. The two were integrated into our CI at the same time, and I mistakenly was considering them one-and-the-same.

The behavior that I recall seems to match something like this: coverallsapp/github-action#37

Where the "change" in code coverage didn't seem to be comparing things in a way that made sense to me.

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

No branches or pull requests

4 participants