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
>
