Hi David, +1 on that strategy.
I see several flaky tests that aren't marked with @Tag("integration") or @IntegrationTest, and I think those would make using the unitTest target ineffective here. We could also start a new tag @Tag("flaky") and exclude that. Thanks, Greg On Fri, Feb 9, 2024 at 8:57 AM David Arthur <mum...@gmail.com> wrote: > > I do think we can add a PR to the merge queue while bypassing branch > potections (like we do for the Merge button today), but I'm not 100% sure. > I like the idea of running unit tests, though I don't think we have data on > how long just the unit tests run on Jenkins (since we run the "test" target > which includes all tests). I'm also not sure how flaky the unit test suite > is alone. > > Since we already bypass the PR checks when merging, it seems that adding a > required compile/check step before landing on trunk is strictly an > improvement. > > What about this as a short term plan: > > 1) Add the merge queue, only run compile/check > 2) Split our CI "test" job into unit and integration so we can start > collecting data on those suites > 3) Add "unitTest" to merge queue job once we're satisfied it won't cause > disruption > > > > > On Fri, Feb 9, 2024 at 11:43 AM Josep Prat <josep.p...@aiven.io.invalid> > wrote: > > > Hi David, > > I like the idea, it will solve the problem we've seen a couple of times in > > the last 2 weeks where compilation for some Scala version failed, it was > > probably overlooked during the PR build because of the flakiness of tests > > and the compilation failure was buried among the amount of failed tests. > > > > Regarding the type of check, I'm not sure what's best, have a real quick > > check or a longer one including unit tests. A full test suite will run per > > each commit in each PR (these we have definitely more than 8 per day) and > > this should be used to ensure changes are safe and sound. I'm not sure if > > having unit tests run as well before the merge itself would cause too much > > of an extra load on the CI machines. > > We can go with `gradlew unitTest` and see if this takes too long or causes > > too many delays with the normal pipeline. > > > > Best, > > > > On Fri, Feb 9, 2024 at 4:16 PM Ismael Juma <m...@ismaeljuma.com> wrote: > > > > > Hi David, > > > > > > I think this is a helpful thing (and something I hoped we would use when > > I > > > learned about it), but it does require the validation checks to be > > reliable > > > (or else the PR won't be merged). Sounds like you are suggesting to skip > > > the tests for the merge queue validation. Could we perhaps include the > > unit > > > tests as well? That would incentivize us to ensure the unit tests are > > fast > > > and reliable. Getting the integration tests to the same state will be a > > > longer journey. > > > > > > Ismael > > > > > > On Fri, Feb 9, 2024 at 7:04 AM David Arthur <mum...@gmail.com> wrote: > > > > > > > Hey folks, > > > > > > > > I recently learned about Github's Merge Queue feature, and I think it > > > could > > > > help us out. > > > > > > > > Essentially, when you hit the Merge button on a PR, it will add the PR > > > to a > > > > queue and let you run a CI job before merging. Just something simple > > like > > > > compile + static analysis would probably save us from a lot of > > headaches > > > on > > > > trunk. > > > > > > > > I can think of two situations this would help us avoid: > > > > * Two valid PRs are merged near one another, but they create a code > > > > breakage (rare) > > > > * A quick little "fixup" commit on a PR actually breaks something (less > > > > rare) > > > > > > > > Looking at our Github stats, we are averaging under 40 commits per > > week. > > > > Assuming those primarily come in on weekdays, that's 8 commits per day. > > > If > > > > we just run "gradlew check -x tests" for the merge queue job, I don't > > > think > > > > we'd get backlogged. > > > > > > > > Thoughts? > > > > David > > > > > > > > > > > > > > > > > > > > -- > > > > David Arthur > > > > > > > > > > > > > -- > > [image: Aiven] <https://www.aiven.io> > > > > *Josep Prat* > > Open Source Engineering Director, *Aiven* > > josep.p...@aiven.io | +491715557497 > > aiven.io <https://www.aiven.io> | <https://www.facebook.com/aivencloud > > > > > <https://www.linkedin.com/company/aiven/> < > > https://twitter.com/aiven_io> > > *Aiven Deutschland GmbH* > > Alexanderufer 3-7, 10117 Berlin > > Geschäftsführer: Oskari Saarenmaa & Hannu Valtonen > > Amtsgericht Charlottenburg, HRB 209739 B > > > > > -- > David Arthur