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

Reply via email to