Also, on the mockito stubbings point, we did upgrade to Mockito 5.8 for the Java 11 and newer builds:
https://github.com/apache/kafka/blob/trunk/gradle/dependencies.gradle#L64 So, we should be good when it comes to that too. Ismael On Fri, Feb 9, 2024 at 4:15 PM Ismael Juma <m...@ismaeljuma.com> wrote: > Nice! > > Ismael > > On Fri, Feb 9, 2024 at 3:43 PM Greg Harris <greg.har...@aiven.io.invalid> > wrote: > >> 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 >> > > >> >
