-
Notifications
You must be signed in to change notification settings - Fork 558
various improvements resulting from reading Testing with CI #2593
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
Conversation
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, a few nits
src/tests/ci.md
Outdated
specify its job name in a job pattern (explained below). | ||
|
||
If you want to run custom CI job(s) in a try build and make sure that they pass all tests and do | ||
If you want to run custom CI job in a try build and make sure that they pass all tests and do |
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.
Nit: the previous wording I find more accurate, because you can indeed run multiple custom CI jobs (in parallel)
Unfortunately, testing a single PR at a time, combined with our long CI (~2 | ||
hours for a full run), means we can’t merge enough PRs in a single day, and a | ||
single failure greatly impacts our throughput. The maximum number of |
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.
Nit: I find the previous "too many" wording is more accurate than "enough"
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.
changing it to "a lot"
[rust-toolstate]: https://rust-lang-nursery.github.io/rust-toolstate | ||
[toolstate documentation]: https://forge.rust-lang.org/infra/toolstate.html | ||
|
||
## Public CI dashboard |
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.
Note: we should probably get rid of this entire section.
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.
why so
133541e
to
6700ed7
Compare
r? ghost