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

Fuzzing runs first #2385

Open
tcharding opened this issue Jan 23, 2024 · 6 comments
Open

Fuzzing runs first #2385

tcharding opened this issue Jan 23, 2024 · 6 comments

Comments

@tcharding
Copy link
Member

#2353 has as one of its aims making CI run faster so it fails quicker but I noticed that the fuzz tests run first and other jobs get queued up behind them. Would it not give us better fuzz coverage if we ran the fuzz test once a day for an hour or so instead of running them every PR for 30 seconds?

@Kixunil
Copy link
Collaborator

Kixunil commented Jan 23, 2024

It did help me actually since we have some parts that are only tested by fuzz, not normal tests unfortunately.

But this is also an issue. It'd be great to de-prioritize it but I haven't found a sane way to do that. At best we could make it "require" a different job but set if to ignore that the previous job failed.

@apoelstra
Copy link
Member

If stuff is getting queued rather than running in parallel we should re-order them so the fast stuff goes first. So first clippy/check (maybe with 2 or 3 feature combinations), then the normal tests, then fuzzing. That would help a lot.

As for "should we move fuzzing to a daily thing rather than every PR" ... I think if fuzzing ran last, it would be ok. It might delay merging, in theory, to have to wait for fuzzing, but at least it won't delay the more-commonly-failing parts of CI.

@dpc
Copy link
Contributor

dpc commented Jan 23, 2024

BTW. Not sure if you looked into it, but GH has these Megue Queues now, and they allows running one set of things on every PR/push, and other right before merging. E.g. In Fedimint we. skip MacOS builds on PRs (because it takes forever), and run it only in the Merge Queue (because we don't want master to be broken on MacOS). Another thing it does is it can possibly run a given CI-step with multiple PRs together once (if you happen to approve lots of them at the same time, which is relatively uncommon, but worth mentioning).

So you could run a proper & longer fuzzing session after PR was approved, when the time it takes is less important, while get a fast ✔️ to unblock reviews etc.

@yancyribbens
Copy link
Contributor

BTW. Not sure if you looked into it, but GH has these Megue Queues now, and they allows running one set of things on every PR/push, and other right before merging.

Unrelated to this PR, however maybe running cargo +nightly fmt right before merging would be better than the current fmt once per week strategy.

@Kixunil
Copy link
Collaborator

Kixunil commented Jan 24, 2024

@yancyribbens not really, it increases the probability of conflicts by pointlessly changing things. Though not doing it sucks too for different reasons. I wrote this: https://github.com/Kixunil/rust-merge which may improve the situation somewhat but it's far from perfect.

@apoelstra
Copy link
Member

Unrelated to this PR, however maybe running cargo +nightly fmt right before merging would be better than the current fmt once per week strategy.

Aside from the logistical issues, we can't do this anyway because merge commits are signed, so Github can't modify the code out from under us even if we requested it.

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

5 participants