Hey Ismael,
I took David's statement here:
> Split our CI "test" job into unit and integration so we can start collecting
> data on those suites
to include moving all the flaky tests to the integrationTest target by
adding the annotation. We can do that while the merge queue is just
running the compile/jar target, and then tweak the merge queue to run
the unitTest target once we're satisfied that it isn't flaky.
Thanks,
Greg
On Fri, Feb 9, 2024 at 9:17 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
