geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Helena Bales <hba...@pivotal.io>
Subject Re: [DISCUSS] Blocking merge button in PR
Date Tue, 22 Oct 2019 18:45:11 GMT
To Bruce's point:

I don't see stress new test as something that's supposed to be easy to
pass. I do agree that it should be easy to figure out what went wrong, but
the point of this task was the catch issues with tests being flaky or
non-repeatable for other reasons. If we reduce the scope of this to only
test newly added test classes, we won't catch the case where someone adds a
new test method that is flaky, or the case where modifying a test method
makes it flaky. Both of these things commonly happen in our code base.
Regarding tests that can't be repeated, this is because they are using an
old test base or pollute their environment. I see both of these as reasons
to go modify the test to make it work properly. Some tests are a lot of
work to change, but having done this many times with tests that I've
touched, I believe it is worthwhile. If we are touching a test class that
does not meet our current standards for testing, then it should be
modernized. If only for the purpose of reducing the flakiness in our
pipelines.

In short, while stressNewTest is a pain, the current design of this tool
serves several purposes, which I believe to be worth the effort of
modernizing outdated tests when we encounter them.

On Tue, Oct 22, 2019 at 11:25 AM Bruce Schuchardt <bschuchardt@pivotal.io>
wrote:

> I also have my doubts about StressNewTest testing being required to
> pass.  Maybe if they just tested new tests instead of anything your
> changes happened to touch it would be easier to pass this check.
>
> Also, the StressNewTest check runs the selected tests in parallel and
> apparently in the same environment.  Any time I've tried to use
> artifacts from one of these runs to figure out what went wrong I found
> them to be useless because of the interleaving of output.
>
>
> On 10/22/19 8:33 AM, Darrel Schneider wrote:
>
> > I would advise against including "StressNewTestOpenJDK11".
> > I have had trouble with this one when doing something as simple as a
> method
> > rename.
> > Because that method was called from an old test, StressNewTest ran that
> old
> > test many times. Not all of our current tests can handle being rerun many
> > times. If all you are trying to do in a pull request is something simple
> > you should not be required to rework a test to survive StressNewTest.
> > If StressNewTest was changed to only run brand new test files then I
> would
> > be okay with it gating a PR merge.
> >
> > On Mon, Oct 21, 2019 at 4:41 PM Bruce Schuchardt <bschuchardt@pivotal.io
> >
> > wrote:
> >
> >> I'd still like to see the PR rerun tool or an equivalent available to
> >> everyone.  Sure you can push an empty commit but that reruns everything,
> >> but the PR tool lets you rerun only the checks that failed.
> >>
> >> On 10/21/19 3:04 PM, Ernest Burghardt wrote:
> >>> +1
> >>> @Naba thanks for seeing this through!
> >>>
> >>> On Mon, Oct 21, 2019 at 1:51 PM Nabarun Nag <nnag@apache.org> wrote:
> >>>
> >>>> Thank you all for all the valuable feedback. Robert was kind enough
to
> >>>> check the technical feasibility of the integration of Github Branch
> >>>> Protection Rules and its compatibility with our concourse CI checks
> and
> >> we
> >>>> are satisfied with the settings provided and the initial tests that
> >> Robert
> >>>> did with a demo geode branch. Please find attached the screenshot of
> the
> >>>> settings that will be sent to INFRA for enabling it on the Apache
> Geode
> >>>> repository.
> >>>>
> >>>> Regards
> >>>> Nabarun
> >>>>
> >>>> On Mon, Oct 21, 2019 at 12:21 PM Aaron Lindsey <alindsey@pivotal.io>
> >>>> wrote:
> >>>>
> >>>>> +1
> >>>>>
> >>>>> - Aaron
> >>>>>
> >>>>>
> >>>>> On Mon, Oct 21, 2019 at 11:53 AM Udo Kohlmeyer <udo@apache.com>
> wrote:
> >>>>>
> >>>>>> BIG +1 (Yes, I'm changing my -1)
> >>>>>>
> >>>>>> @Naba, thank you for the offline chat. It seems that the proposal
of
> >>>>>> Github enforcing our good practices is a great option.
> >>>>>>
> >>>>>> 2 merged PR's without a full green pipeline, since 18 Oct 2019,
> >> shocking
> >>>>>> and disgusting!!
> >>>>>>
> >>>>>> I would just like to reiterate to ALL committers, you have a
> >>>>>> responsibility towards the project to commit/merge only the
best
> code
> >>>>>> possible. If things break, fix them. Don't merge and hope that
it
> goes
> >>>>>> away and someone else fixes it.
> >>>>>>
> >>>>>> I really don't want to support a mechanism where the project
has
> >> become
> >>>>>> so prescriptive and restrictive, all because we have a few
> committers
> >>>>>> who will not follow the established and agreed processes. BUT,
once
> >>>>>> again, the masses are impacted due to a few.
> >>>>>>
> >>>>>> Thank you Naba for following this through.
> >>>>>>
> >>>>>> --Udo
> >>>>>>
> >>>>>> On 10/21/19 11:05 AM, Nabarun Nag wrote:
> >>>>>>> @Karen
> >>>>>>>
> >>>>>>> Thank you so much for the feedback. I understand that committers
> must
> >>>>>> trust
> >>>>>>> each other and I agree entirely with that. This proposal
is just
> >>>>>> preventing
> >>>>>>> us from making mistakes. Its just guard rails. As you can
see in
> the
> >>>>>> email
> >>>>>>> chain that this mistake was made multiple times and this
has let
> to a
> >>>>> lot
> >>>>>>> of engineers give up their time and detecting and fixing
this
> issue.
> >>>>> We
> >>>>>>> still trust our committers, we are just enabling some features
to
> >>>>> avoid
> >>>>>>> time being wasted on avoidable mistakes and use that time
in
> >>>>>>> improving Geode. I hope I can persuade you to change your
opinion.
> >>>>>>>
> >>>>>>> @Owen
> >>>>>>> Thank you for accepting the baby step proposal. Yes, let's
see in
> >> some
> >>>>>> time
> >>>>>>> if this is successful. We can always undo this if needed.
> >>>>>>>
> >>>>>>> @Rest
> >>>>>>> - Blaming people etc. will be very detrimental to the community,
I
> do
> >>>>> not
> >>>>>>> propose that.
> >>>>>>> - This proposal was not just my idea but from feedback from
lot of
> >>>>>>> developers who contribute to Geode on a frequent basis.
> >>>>>>> - It is very* unfair *to the engineers to go and investigate
and
> find
> >>>>> out
> >>>>>>> what caused the failure and then send emails etc, their
time can be
> >>>>> used
> >>>>>> in
> >>>>>>> doing something more valuable.
> >>>>>>> - This is not something new, we have had this issue for
over 3
> years
> >>>>> now,
> >>>>>>> and maintaining this as the status quo is not healthy. Let
us try
> >> this
> >>>>>>> solution, the committers, and developers who contribute
on a
> regular
> >>>>>> basis
> >>>>>>> will immediately see if it works or does not work and can
revert it
> >> if
> >>>>>> this
> >>>>>>> does not.
> >>>>>>> - There is a problem and this is an attempt at the solution.
If it
> >>>>> does
> >>>>>> not
> >>>>>>> work, we can revert it. For the sake of all the developers
who are
> in
> >>>>>> pain
> >>>>>>> and helping them spend the time on more productive tasks,
let us
> just
> >>>>>> give
> >>>>>>> this proposal a chance. If there are any issues we can revert
it.
> >>>>>>>
> >>>>>>>
> >>>>>>> *Reiterating the proposal:*
> >>>>>>> Github branch protection rule for :
> >>>>>>> - at least one review
> >>>>>>> - Passing build, unit and stress test.
> >>>>>>>
> >>>>>>>
> >>>>>>> In our opinion, no committer would want to check-in code
with
> failing
> >>>>> any
> >>>>>>> of the above.
> >>>>>>>
> >>>>>>> Regards
> >>>>>>> Nabarun
> >>>>>>>
> >>>>>>> On Mon, Oct 21, 2019 at 10:28 AM Alexander Murmann <
> >>>>> amurmann@apache.org>
> >>>>>>> wrote:
> >>>>>>>
> >>>>>>>> Udo, I think you are making a great point! I am fully
bought into
> >>>>>> trusting
> >>>>>>>> our committers to know how many reviews are needed for
their PRs.
> We
> >>>>>> should
> >>>>>>>> be able to have the same trust into knowing when it's
OK to merge.
> >>>>> If a
> >>>>>>>> committer wants to they can after all, always merge
and push
> without
> >>>>> a
> >>>>>> PR.
> >>>>>>>> That's because we trust them.
> >>>>>>>>
> >>>>>>>> If we are seeing that our committers merge without sufficient
> review
> >>>>>> (via
> >>>>>>>> human or automated means), is this an education problem
we can
> >> solve?
> >>>>>>>> On Mon, Oct 21, 2019 at 10:18 AM Udo Kohlmeyer <udo@apache.com>
> >>>>> wrote:
> >>>>>>>>> I must agree with @Karen here..
> >>>>>>>>>
> >>>>>>>>> All committers are trusted to do the right thing.
One of those
> >>>>> things
> >>>>>> is
> >>>>>>>>> to commit (or not commit) PR's.
> >>>>>>>>>
> >>>>>>>>> Now we are discussing disabling the button when
tests are
> failing.
> >>>>> Why
> >>>>>>>>> stop there? Why not, that the submitter of the said
commit does
> not
> >>>>> get
> >>>>>>>>> to merge their own PR?
> >>>>>>>>>
> >>>>>>>>> Now, that of course is taking it to the extreme,
that we don't
> want
> >>>>> (at
> >>>>>>>>> least I don't want to be THAT over prescriptive)..
So why do we
> >>>>> want to
> >>>>>>>>> now limit when I can merge? It remains the committers
> >>>>> responsibility to
> >>>>>>>>> merge commits into the project, that are of the
expected quality.
> >>>>> IF it
> >>>>>>>>> so happens that one, by accident, has merged a PR
before it was
> >>>>> green,
> >>>>>>>>> revert it. All committers have the power to do so.
> >>>>>>>>>
> >>>>>>>>> So from my perspective, a -1 on disabling the Merge
button, just
> >>>>>> because
> >>>>>>>>> someone was not careful in merging and without following
our
> >>>>> protocol
> >>>>>> of
> >>>>>>>>> waiting for an "All green".
> >>>>>>>>>
> >>>>>>>>> --Udo
> >>>>>>>>>
> >>>>>>>>> On 10/21/19 10:11 AM, Ernest Burghardt wrote:
> >>>>>>>>>> +1 to enacting this immediately... just this
weekend a PR was
> >>>>> merged
> >>>>>>>> with
> >>>>>>>>>> failures on all of the following:
> >>>>>>>>>> * concourse-ci/DistributedTestOpenJDK11 * Concourse
CI build
> >>>>> failure
> >>>>>>>>>> * concourse-ci/UnitTestOpenJDK11 * Concourse
CI build failure
> >>>>>>>>>> * concourse-ci/UnitTestOpenJDK8 * Concourse
CI build failure
> >>>>>>>>>> * concourse-ci/UpgradeTestOpenJDK11 * Concourse
CI build failure
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Thanks,
> >>>>>>>>>> EB
> >>>>>>>>>>
> >>>>>>>>>> On Mon, Oct 21, 2019 at 10:01 AM Karen Miller
<
> kmiller@pivotal.io
> >>>>>>>>> wrote:
> >>>>>>>>>>> I have (more than once) committed docs changes
for typo fixes
> >>>>> without
> >>>>>>>>>>> review.  I generally label the commits
> >>>>>>>>>>> with a bold "Commit then Review" message.
 But, I am bringing
> >>>>> this up
> >>>>>>>>> as I
> >>>>>>>>>>> have purposely not followed what
> >>>>>>>>>>> looks to be a positively-received proposed
policy, since I have
> >>>>> not
> >>>>>>>>> gotten
> >>>>>>>>>>> reviews. If all feel that we need a
> >>>>>>>>>>> rule for everyone to follow (instead of
a guideline that PRs
> >> shall
> >>>>>>>> have
> >>>>>>>>> at
> >>>>>>>>>>> least 1 review), I will follow the rule,
> >>>>>>>>>>> but I'm a -0 on the process. I get it, and
I understand its
> >>>>> purpose
> >>>>>>>> and
> >>>>>>>>>>> intent, but I personally prefer to trust
that each
> >>>>>>>>>>> comitter takes personal responsibility for
the code they commit
> >>>>> WRT
> >>>>>>>>> waiting
> >>>>>>>>>>> for tests and/or obtaining reviews.
> >>>>>>>>>>> Karen
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> On Mon, Oct 21, 2019 at 6:24 AM Joris Melchior
<
> >>>>> jmelchior@pivotal.io
> >>>>>>>>>>> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>> +1 to the revised approach. I think
requiring at least one
> >>>>> review is
> >>>>>>>>>>>> important. More eyes make for better
code.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Cheers, Joris.
> >>>>>>>>>>>>
> >>>>>>>>>>>> On Mon, Oct 21, 2019 at 8:11 AM Ju@N
<jujoramos@gmail.com>
> >>>>> wrote:
> >>>>>>>>>>>>> +10 to Naba's proposal, it seems
the right thing to do and
> will
> >>>>>> help
> >>>>>>>>> us
> >>>>>>>>>>>> to
> >>>>>>>>>>>>> prevent accidentally breaking *develop*
while keeping focus
> on
> >>>>>>>> people
> >>>>>>>>>>>>> instead of processes.
> >>>>>>>>>>>>> I'd add, however, that the *Merge
Pull Request* button should
> >>>>>> remain
> >>>>>>>>>>>>> disabled until *all CIs have finished*,
and only enable it
> once
> >>>>> the
> >>>>>>>>>>>> *Build,
> >>>>>>>>>>>>> Unit, Stress Tests and LGTM are
green *(that is, force the
> >>>>>> committer
> >>>>>>>>> to
> >>>>>>>>>>>>> wait at least until all CIs are
done)*. *I also agree in that
> >>>>> that
> >>>>>>>> we
> >>>>>>>>>>>>> should require *at least one* official
approval.
> >>>>>>>>>>>>> Cheers.
> >>>>>>>>>>>>>
> >>>>>>>>>>>> --
> >>>>>>>>>>>> *Joris Melchior *
> >>>>>>>>>>>> CF Engineering
> >>>>>>>>>>>> Pivotal Toronto
> >>>>>>>>>>>> 416 877 5427
> >>>>>>>>>>>>
> >>>>>>>>>>>> “Programs must be written for people
to read, and only
> >>>>> incidentally
> >>>>>>>> for
> >>>>>>>>>>>> machines to execute.” – *Hal Abelson*
> >>>>>>>>>>>> <https://en.wikipedia.org/wiki/Hal_Abelson>
> >>>>>>>>>>>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message