-
Notifications
You must be signed in to change notification settings - Fork 435
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
Fix Codecov measuring and Publishing #863
Conversation
Causes a linker error when building `08-flipper-as-dependency-trait` due to `__ink_enforce_error` otherwise.
Regarding the linker failure:
|
dropping |
sadly |
…k into 3x8_fix_codecov_publishing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-Clink-dead-code is correct. The flipper example actually should just not generate the __ink_enforce_error in case it is a success test OR it should not be put under coverage if it is an enforced failure.
OK, now it works properly. |
Now, I'm not an expert in all those flags and in fact I wanted you guys to take a look at the quality of the coverage and come up with the decision whether it's good enough (better than previous). |
Did some research See https://github.com/taiki-e/cargo-llvm-cov#continuous-integration and notes below
Also rust-lang/rust#86177
And a tracking issue about source-based coverage rust-lang/rust#79121 All of it means that the current state of the PR means that we are thrown back to line coverage. |
Working MIR-based coverage lies in the commit above. However, it does not provide branches coverage and a lot of effort was put into it in ink!, so it was decided to return to the previous instrumentation. |
@TriplEight Can we close this PR, since we've merged #895 now? |
yes, but I'll note it for myself as a working example of the source-based coverage. |
Apparently, after their security issue, Codecov decided to fix up some security. Now they require tokens even for public projects.
As for failing after the script, it used to before, but it's on their side, so now it does 👎 . This is fixed with
-Z
option.UPD: in fact, they've released a safer option to upload the test cov results, a binary. paritytech/scripts#318
UPD2: which does not actually work so far.
CC: #857
Closes: https://github.com/paritytech/ci_cd/issues/160