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

chore: add test coverage reporting job to CI #262

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

greenhat
Copy link
Contributor

@greenhat greenhat commented Jul 29, 2024

Close #245

This PR updates the rust to the nightly-2024-05-15 (the later versions brake the time crate build).

The pinned cargo-component version (0.7.0) was too old for this rustc, so I have to update it to the latest 0.14.0 which introduced quite many breaking changes (separate wit-bindgen-rt crate, export! macro), so I have to update all our Wasm CM tests.

The test coverage job only upload coverage XML report generated by cargo-tarpoulin to coveralls.io. I'll add a fancy report (HTML, etc.) in a separate PR.

TODO:

  • Extract code coverage changes into a separate PR and close this one.

@greenhat greenhat force-pushed the greenhat/i245-ci-test-cov branch 5 times, most recently from 4149074 to 5c098e7 Compare July 30, 2024 16:17
@greenhat
Copy link
Contributor Author

greenhat commented Jul 31, 2024

@bobbinth I requested access for https://coveralls.io to this repo the org (read permissions) from the repo org owners. This is needed for test coverage reports. Thanks.

EDIT: I requested access to the org since there is no more granular per-repo access.

@greenhat greenhat force-pushed the greenhat/i245-ci-test-cov branch 4 times, most recently from c31a0d2 to 173bfff Compare July 31, 2024 13:05
@bobbinth
Copy link
Contributor

bobbinth commented Aug 2, 2024

@bobbinth I requested access for https://coveralls.io to this repo the org (read permissions) from the repo org owners. This is needed for test coverage reports. Thanks.

EDIT: I requested access to the org since there is no more granular per-repo access.

It seems like even without the permission it should be able to read public data in the repo (from what I understand, the permission is needed to read private data). Since we don't really have private data, could it work without the permission?

@greenhat
Copy link
Contributor Author

greenhat commented Aug 5, 2024

It seems like even without the permission it should be able to read public data in the repo (from what I understand, the permission is needed to read private data). Since we don't really have private data, could it work without the permission?

Their GitHub integration is done through the app and the first step is to add the compiler repo in my account on coveralls.io (see their guide). I don't see 0xPolygonMiden repos in my coveralls.io account, so I guess their GitHub app needs the access to the org to fetch the org repos. After that, their GitHub app needs to be authorized to access the repo and post the coverage data in the PR comments. I wonder if this process will be easier/different for your coveralls.io account as the owner of the compiler repo. Could you please try to register with coveralls.io (login with your GitHub account) and add the compiler repo to your coveralls account?

@greenhat greenhat marked this pull request as ready for review August 6, 2024 13:44
@greenhat greenhat requested a review from bitwalker August 6, 2024 13:44
args = ["install", "cargo-component@0.7.0"]
args = ["install", "cargo-component@0.14.0"]

[tasks.install-cargo-tarpaulin]
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't actually need this task, if you have a task that uses cargo as the command, cargo-make will check if the task name passed to cargo exists, and if not, it will attempt to cargo install it.

There's also cargo-make configuration for modifying how cargo install is invoked, like if the crate name is different, or additional arguments are needed. Not needed here, but just FYI.

The only reason to explicitly install things is when those things aren't normal cargo plugins, e.g. mdbook

"lcov",
]
dependencies = [
"install-wasm-target",
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we use rust-toolchain.toml, it occurs to me that we don't need to explicitly install the targets/components anymore, we just need to make sure they are in the toolchain file.

Copy link
Contributor

Choose a reason for hiding this comment

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

To make merging my bitwalker/rodata-init branch on top of this relatively painless, could you apply the same set of changes to this file (and their related changes in emitter.rs) as I did? It's basically the same changes, but I went a step further with trimming out unused stuff.

Copy link
Contributor

@bitwalker bitwalker left a comment

Choose a reason for hiding this comment

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

Looks good! A few suggested changes, but once those are made, feel free to go ahead and merge this

Copy link
Contributor

@bitwalker bitwalker left a comment

Choose a reason for hiding this comment

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

So I was working on things today, and actually got the test suite down to only 3 tests failing - and while I was reviewing this again to see where our changes conflict, I came to the conclusion that it is likely simpler for you to apply just the test coverage and cargo-component-related changes to bitwalker/rodata-init (soon to be next), rather than the reverse.

This is because not only does my branch contain the toolchain changes, and a bunch of test fixes that this also contains, the conflicts are all in stuff that we both ended up doing independently on our branches, with the exception of the tests affected by the cargo-component changes, which just need to be updated as normal anyway.

Let me know what you think

@greenhat
Copy link
Contributor Author

greenhat commented Aug 7, 2024

So I was working on things today, and actually got the test suite down to only 3 tests failing - and while I was reviewing this again to see where our changes conflict, I came to the conclusion that it is likely simpler for you to apply just the test coverage and cargo-component-related changes to bitwalker/rodata-init (soon to be next), rather than the reverse.

I agree. I'll make separate PRs to the next branch with test coverage and cargo-component shortly.

@greenhat
Copy link
Contributor Author

greenhat commented Aug 7, 2024

So I was working on things today, and actually got the test suite down to only 3 tests failing

The next branch still has 127 tests failing. Could you push your changes?

@greenhat greenhat marked this pull request as draft August 7, 2024 03:25
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.

Generate a test coverage report on CI
3 participants