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 > > > >
