ci: setup miri #74
ci: setup miri #74
Conversation
seqre
force-pushed
the
miri
branch
3 times, most recently
from
584e681
to
6f57813
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Flags with carried forward coverage won't be shown. Click here to find out more.
🚨 Try these New Features:
|
| run: cargo miri setup | ||
|
|
||
| - name: Miri test | ||
| run: cargo miri nextest run -E 'not (test(db) | test(password_hash))' --no-fail-fast --all-features |
There was a problem hiding this comment.
Why are we handling this differently than other tests? We could just add #![cfg_attr(miri, ignore)] at the top of db.rs and annotate the password_hash* tests with #[cfg_attr(miri, ignore)]. This will make it easier for people wanting to run Miri tests locally instead of using the CI.
As about why password_hash tests do not work on Miri, my guess is they're just super slow. They can easily take 500ms+ on a normal debug build without Miri, so with Miri it can be easily magnified by 1000x or more. There's not much value in running these tests anyway, as this really just calls external code and does little above that.
There was a problem hiding this comment.
It's kind of sad the tests are taking 8 minutes+ to run, but it is what it is, I guess. Hopefully they won't be too annoying, but given we don't use any unsafe code (for now), I would assume that if the normal tests pass, Miri tests will pass as well (except for the sqlite stuff, which is expected).
LGTM, but please address the comment in rust.yml.
@m4tx, I thought about reducing the number of times it runs, maybe just to tags, considering the type of testing it is. What do you think? |
|
@seqre I think we can keep it for pushes/PRs for now, as long as a) we don't require them to pass to merge PRs and b) |
dbandpassword_hashtests would not finish, investigating why can be a future endeavor