Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest
At least one person on the thread (@anthony) raised concerns but has not replied since. Also since this thread was started, the bug that miscounted files has been fixed, which is sufficient that stress tests would have been run for about 95% of PRs that previously (erroneously) were given a free pass. Maybe that is enough? I’m hesitant to raise the bar for a required check if it will become an added hardship, and especially hesitant to create a situation where the highly-unpopular “manual override” might be the only option. It might be better to just make slight increases from 25 files/6 hours to 35 files/10 hours, and just ask that if anyone is touching more test files than that (due to a refactor/rename/reformat) to please make that a separate PR separate from any actual code changes. This is better for reviewers too... Discussion period goes through the end of this week. On Tue, Mar 17, 2020 at 8:55 AM Dan Smith wrote: > Seems like I'm the only one on this thread who even quibbled about removing > the limit entirely. Let's go ahead and remove the limit. > > -Dan > > On Fri, Mar 13, 2020 at 8:26 AM Robert Houghton > wrote: > > > I want the check to stay required for PR merges to be allowed. I also > want > > it to do real work in all cases. If I can add whitespace to 25 test > classes > > and get a free pass, thats a big testing hole. > > > > Remove the limit. Worst case, we have to A) bump timeouts in the future > B) > > put the limits back in C) come up with a new paradigm for verifying test > > quality in a non-deterministic process > > > > On Tue, Mar 10, 2020 at 8:58 PM Dan Smith wrote: > > > > > I checked, and 1% of our last 500 commits touched more than 30 tests. > Of > > > those 1%, over half touched more than 100 tests. So I'd guess somewhere > > > around .5% of prs will fall under the proposed changes. > > > > > > My preference would be to just raise the threshold given that we > already > > > raised the timeout. That'll catch 99% of prs without making test > > > refactoring and cleanups that touch many tests hard. > > > > > > But, I'm ok with removing the limit if people really feel that a manual > > > override process is better for that 1%. > > > > > > -Dan > > > On Tue, Mar 10, 2020, 4:33 PM Owen Nichols > wrote: > > > > > > > To recap, the original question was: should we change StressNewTest > to > > > > pass ONLY if it is able to successfully run all new/changed tests 50 > > > times > > > > (eliminating the existing loophole exempting PRs that touch 25 or > more > > > > files)? > > > > > > > > So far a handful of people have expressed support, but many have > > > > questions/concerns: > > > > - One concern was mis-counting of test files touched. As of today > this > > > is > > > > now fixed. > > > > - A big concern was that it might become more difficult to pass all > > > > required PR checks. > > > > - Another concern was that the current timeout of 6 hours might be > too > > > > high / too low. > > > > - An alternative was suggested to keep the loophole but maybe > increase > > > the > > > > threshold required to get the free pass. > > > > - If we’re going to raise the bar on any required PR check, maybe we > > > > should consider making it non-required. > > > > > > > > Let’s extend the comment period until the end of next week (Friday > Mar > > > 20) > > > > in hopes of converging on a solution everybody is happy with (even if > > it > > > > isn’t what was originally proposed). > > > > > > > > > > > > > On Mar 6, 2020, at 4:51 PM, Donal Evans > wrote: > > > > > > > > > > With fairly apt timing, we've recently had a PR merged ( > > > > > https://github.com/apache/geode/pull/4745) that introduced a test > > that > > > > has > > > > > resulted in fairly consistently flaky failures in the pipeline > (JIRA > > > > > ticket: > > > https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843 > > > > ). > > > > > The PR was quite large and touched/created a lot of tests, so > > > > StressNewTest > > > > > never ran on it: > > > > > https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2. > > Given > > > > how > > > > > frequently the test is failing in the pipeline (11 failures on > > various > > > > > IntegrationTest jobs over the past 6 commits), it seems very likely > > > that > > > > > had StressNewTest been allowed to run, this issue would have been > > > caught > > > > at > > > > > the PR stage and could have been remedied then, instead of > resulting > > > in a > > > > > nearly constantly red pipeline. > > > > > > > > > > I've already given my +1 to this proposal, but this situation has > > > > prompted > > > > > me to make it a +2. > > > > > > > > > > On Fri, Mar 6, 2020 at 1:46 PM Donal Evans > > wrote: > > > > > > > > > >> With fairly apt timing, we've recently had a PR merged ( > > > > >> https://github.com/apache/geode/pull/4745) that introduced a test > > > that > > > > >> has resulted in fairly consistently flaky failures in the pipeline > > > (JIRA > >
Re: Discussion on Deprecation
> On Mar 17, 2020, at 9:03 AM, John Blum wrote: > > Additionally, it'd be ideal if the deprecated method were then adapted to > delegate to the new approach. This will cut down on the number of required > tests since then you only need a Unit Tests asserting the method performs > the translation/delegating appropriately, unless of course the behavior is > changing too. +1 to delegating
Re: Discussion on Deprecation
Additionally, it'd be ideal if the deprecated method were then adapted to
delegate to the new approach. This will cut down on the number of required
tests since then you only need a Unit Tests asserting the method performs
the translation/delegating appropriately, unless of course the behavior is
changing too.
On Tue, Mar 17, 2020 at 8:57 AM Udo Kohlmeyer wrote:
> I think we are also missing the other side of the coin.
>
> Once we deprecate something and we now need a equivalent test that tests
> the same behavior using the new method/approach. i.e now we have to
> double up on the testing of said deprecated method/feature/class. First
> we have to keep the tests around that use the deprecated and now we need
> the same test for the new. Otherwise we cannot ever be certain that both
> approaches work.
>
> In addition to this, if we don't create both tests, we have to create
> all the tests at the time of removal, otherwise we lose coverage.
>
> --Udo
>
> On 3/16/20 9:27 AM, Joris Melchior wrote:
> > +1 on leaving testing of deprecated functionality in place
> >
> > On Mon, Mar 16, 2020 at 11:50 AM Dan Smith wrote:
> >
> >> +1
> >>
> >> One point though - we do need to leave some tests that specifically test
> >> the deprecated method(s), since we still support the deprecated APIs
> until
> >> we remove them. Everything else should be converted.
> >>
> >> -Dan
> >>
> >> On Sun, Mar 15, 2020 at 6:41 PM Jacob Barrett
> wrote:
> >>
> >>> Hey Team,
> >>>
> >>> When deprecating a symbol, like class or method, please included a
> >>> reference to the replacement in the java docs. Also please include an
> >>> example of converting from the old API to the new. I am finding many
> many
> >>> places in the source where deprecated code has no hints at all. As many
> >> of
> >>> us don’t take the effort to immediately convert old tests over to the
> new
> >>> APIs the context is lost when someone finally does get around to it.
> For
> >>> public APIs this is extremely important so that users know how to
> migrate
> >>> their code with as few roadblocks as possible.
> >>>
> >>> Here is an example of something that is much better than most of what I
> >> am
> >>> seeing.
> >>>
> >>> /**
> >>> * @deprecated Replaced with something better and safer.
> >>> * Replaced by {@link Bar}.
> >>> */
> >>> @Deprecated
> >>> class Foo {
> >>>/**
> >>> * Do something.
> >>> *
> >>> * @deprecated Replaced with something better.
> >>> * Replaced by {@link Bar#doSomethingBetter()}
> >>> */
> >>>@Deprecated
> >>>public void doSomething() {}
> >>>
> >>>/**
> >>> * Do something.
> >>> *
> >>> * @deprecated Implementation is not safe.
> >>> * Replaced with {@link Bar#doSomethingBetter()} and {@link
> >>> Bar#doSomethingElse()}.
> >>> * Migration example, replace:
> >>> * {@code
> >>> * Foo foo = new Foo();
> >>> * foo.doSomethingAndSomethingElse();
> >>> * }
> >>> * with:
> >>> * {@code
> >>> * Bar bar = Bar();
> >>> * bar.doSomethingBetter();
> >>> * bar.doSomethingElse();
> >>> * }
> >>> */
> >>>@Deprecated
> >>>public void doSomethingAndSomethingElse() {}
> >>> }
> >>>
> >>> class Bar {
> >>>public void doSomethingBetter() {}
> >>>public void doSomethingElse() {}
> >>> }
> >>>
> >>> -Jake
> >>>
> >>>
> >
>
--
-John
Spring Data Team
Re: Discussion on Deprecation
I think we are also missing the other side of the coin.
Once we deprecate something and we now need a equivalent test that tests
the same behavior using the new method/approach. i.e now we have to
double up on the testing of said deprecated method/feature/class. First
we have to keep the tests around that use the deprecated and now we need
the same test for the new. Otherwise we cannot ever be certain that both
approaches work.
In addition to this, if we don't create both tests, we have to create
all the tests at the time of removal, otherwise we lose coverage.
--Udo
On 3/16/20 9:27 AM, Joris Melchior wrote:
+1 on leaving testing of deprecated functionality in place
On Mon, Mar 16, 2020 at 11:50 AM Dan Smith wrote:
+1
One point though - we do need to leave some tests that specifically test
the deprecated method(s), since we still support the deprecated APIs until
we remove them. Everything else should be converted.
-Dan
On Sun, Mar 15, 2020 at 6:41 PM Jacob Barrett wrote:
Hey Team,
When deprecating a symbol, like class or method, please included a
reference to the replacement in the java docs. Also please include an
example of converting from the old API to the new. I am finding many many
places in the source where deprecated code has no hints at all. As many
of
us don’t take the effort to immediately convert old tests over to the new
APIs the context is lost when someone finally does get around to it. For
public APIs this is extremely important so that users know how to migrate
their code with as few roadblocks as possible.
Here is an example of something that is much better than most of what I
am
seeing.
/**
* @deprecated Replaced with something better and safer.
* Replaced by {@link Bar}.
*/
@Deprecated
class Foo {
/**
* Do something.
*
* @deprecated Replaced with something better.
* Replaced by {@link Bar#doSomethingBetter()}
*/
@Deprecated
public void doSomething() {}
/**
* Do something.
*
* @deprecated Implementation is not safe.
* Replaced with {@link Bar#doSomethingBetter()} and {@link
Bar#doSomethingElse()}.
* Migration example, replace:
* {@code
* Foo foo = new Foo();
* foo.doSomethingAndSomethingElse();
* }
* with:
* {@code
* Bar bar = Bar();
* bar.doSomethingBetter();
* bar.doSomethingElse();
* }
*/
@Deprecated
public void doSomethingAndSomethingElse() {}
}
class Bar {
public void doSomethingBetter() {}
public void doSomethingElse() {}
}
-Jake
Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest
Seems like I'm the only one on this thread who even quibbled about removing the limit entirely. Let's go ahead and remove the limit. -Dan On Fri, Mar 13, 2020 at 8:26 AM Robert Houghton wrote: > I want the check to stay required for PR merges to be allowed. I also want > it to do real work in all cases. If I can add whitespace to 25 test classes > and get a free pass, thats a big testing hole. > > Remove the limit. Worst case, we have to A) bump timeouts in the future B) > put the limits back in C) come up with a new paradigm for verifying test > quality in a non-deterministic process > > On Tue, Mar 10, 2020 at 8:58 PM Dan Smith wrote: > > > I checked, and 1% of our last 500 commits touched more than 30 tests. Of > > those 1%, over half touched more than 100 tests. So I'd guess somewhere > > around .5% of prs will fall under the proposed changes. > > > > My preference would be to just raise the threshold given that we already > > raised the timeout. That'll catch 99% of prs without making test > > refactoring and cleanups that touch many tests hard. > > > > But, I'm ok with removing the limit if people really feel that a manual > > override process is better for that 1%. > > > > -Dan > > On Tue, Mar 10, 2020, 4:33 PM Owen Nichols wrote: > > > > > To recap, the original question was: should we change StressNewTest to > > > pass ONLY if it is able to successfully run all new/changed tests 50 > > times > > > (eliminating the existing loophole exempting PRs that touch 25 or more > > > files)? > > > > > > So far a handful of people have expressed support, but many have > > > questions/concerns: > > > - One concern was mis-counting of test files touched. As of today this > > is > > > now fixed. > > > - A big concern was that it might become more difficult to pass all > > > required PR checks. > > > - Another concern was that the current timeout of 6 hours might be too > > > high / too low. > > > - An alternative was suggested to keep the loophole but maybe increase > > the > > > threshold required to get the free pass. > > > - If we’re going to raise the bar on any required PR check, maybe we > > > should consider making it non-required. > > > > > > Let’s extend the comment period until the end of next week (Friday Mar > > 20) > > > in hopes of converging on a solution everybody is happy with (even if > it > > > isn’t what was originally proposed). > > > > > > > > > > On Mar 6, 2020, at 4:51 PM, Donal Evans wrote: > > > > > > > > With fairly apt timing, we've recently had a PR merged ( > > > > https://github.com/apache/geode/pull/4745) that introduced a test > that > > > has > > > > resulted in fairly consistently flaky failures in the pipeline (JIRA > > > > ticket: > > https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843 > > > ). > > > > The PR was quite large and touched/created a lot of tests, so > > > StressNewTest > > > > never ran on it: > > > > https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2. > Given > > > how > > > > frequently the test is failing in the pipeline (11 failures on > various > > > > IntegrationTest jobs over the past 6 commits), it seems very likely > > that > > > > had StressNewTest been allowed to run, this issue would have been > > caught > > > at > > > > the PR stage and could have been remedied then, instead of resulting > > in a > > > > nearly constantly red pipeline. > > > > > > > > I've already given my +1 to this proposal, but this situation has > > > prompted > > > > me to make it a +2. > > > > > > > > On Fri, Mar 6, 2020 at 1:46 PM Donal Evans > wrote: > > > > > > > >> With fairly apt timing, we've recently had a PR merged ( > > > >> https://github.com/apache/geode/pull/4745) that introduced a test > > that > > > >> has resulted in fairly consistently flaky failures in the pipeline > > (JIRA > > > >> ticket: > > https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843 > > > ). > > > >> The PR was quite large and touched/created a lot of tests, so > > > StressNewTest > > > >> never ran on it: > > > >> https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2. > > Given > > > >> how frequently the test is failing in the pipeline (11 failures on > > > various > > > >> IntegrationTest jobs over the past 6 commits), it seems very likely > > that > > > >> had StressNewTest been allowed to run, this issue would have been > > > caught at > > > >> the PR stage and could have been remedied then, instead of resulting > > in > > > a > > > >> nearly constantly red pipeline. > > > >> > > > >> I've already given my +1 to this proposal, but this situation has > > > prompted > > > >> me to make it a *+1*. > > > >> > > > >> On Tue, Mar 3, 2020 at 2:08 PM Anilkumar Gingade < > aging...@pivotal.io > > > > > > >> wrote: > > > >> > > > >>> The stress test is to identify the flaky-ness within the test; and > > > >>> assuming > > > >>> any changes to the test may have introduced the flaky-ness. > > > >>> It's about paying the cost upfront or later when
Odg: Next release
Thanks for the info! BR, Mario Šalje: Ernest Burghardt Poslano: 17. ožujka 2020. 0:06 Prima: dev@geode.apache.org Predmet: Re: Next release Hi Mario, There is still some work to be done to ensure performance is on par with previous releases... here are a few tickets related to the efforts https://issues.apache.org/jira/browse/GEODE-7763 https://issues.apache.org/jira/browse/GEODE-7832 https://issues.apache.org/jira/browse/GEODE-7853 https://issues.apache.org/jira/browse/GEODE-7863 https://issues.apache.org/jira/browse/GEODE-6154 Best regards, EB On Mon, Mar 16, 2020 at 2:03 AM Mario Kevo wrote: > Hi geode-dev, > > When we will have Apache Geode 1.12.0 release? > I saw that all test passed and the last commit was before 4 days(typos > correct). > Are we waiting fix for some critical issue or something else? > > Thanks and BR, > Mario >
