Hey all, I implemented a fairly aggressive PR [1] to demote flaky tests to integration tests, and the end result is a much faster (10m locally, 1h on Jenkins) build which is also very reliable.
I believe this would make unitTest suitable for use in the merge queue, with the caveat that it doesn't run 25k integration tests, and doesn't perform the mockito strict stubbing verification. This would still be a drastic improvement, as we would then be running the build and 87k unit tests that we aren't running today. Thanks! Greg [1] https://github.com/apache/kafka/pull/15349 On Fri, Feb 9, 2024 at 9:25 AM Ismael Juma <m...@ismaeljuma.com> wrote: > > Please check https://github.com/apache/kafka/pull/14186 before making the > `unitTest` and `integrationTest` split. > > Ismael > > On Fri, Feb 9, 2024 at 9:16 AM Josep Prat <josep.p...@aiven.io.invalid> > wrote: > > > Regarding "Split our CI "test" job into unit and integration so we can > > start collecting data on those suites", can we run these 2 tasks in the > > same machine? So they won't need to compile classes twice for the same > > exact code? > > > > On Fri, Feb 9, 2024 at 6:05 PM Ismael Juma <m...@ismaeljuma.com> wrote: > > > > > Why can't we add @Tag("integration") for all of those tests? Seems like > > > that would not be too hard. > > > > > > Ismael > > > > > > On Fri, Feb 9, 2024 at 9:03 AM Greg Harris <greg.har...@aiven.io.invalid > > > > > > wrote: > > > > > > > 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 > > > > > > > > > > > > > -- > > [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 > >
