I think it would be a mistake to have just the main pipeline catch analyze-serializable failures. It is easy to accidentally cause these failures so I think we want them caught early. They are also not flaky so if broken then they will consistently fail. I think this would cause someone extra work in tracking down the change, backing it out, and resubmitting the PR once fixed. I would vote for getting these checks back into PR checks as quickly as possible. That is probably "revert PR 3598". Then someone who knows these tests well can propose the best way to improve them. But after reverting you could also resubmit it with something like #2 or #3 that runs the existing tests that require JDK 8 but not all the other tests.
On Sun, May 19, 2019 at 2:20 AM Owen Nichols <onich...@pivotal.io> wrote: > Correct, analyze-serializables test is currently skipped under JDK11. > Ideally it would be re-written to test what is actually serialized, not the > flawed assumption that no change in compiled bytecode size means nothing > changed... > > This also brings up a good question worth discussing: is the goal of PR > checks to catch all of the same issues as the main pipeline, or is it ever > ok to run some tests only in the main pipeline (for example, PR checks have > never included Windows tests). > > So which is the preferred course of action? > 1) restore ALL JDK8 tests to PR checks (i.e. revert PR 3598) > 2) restore just the one JDK8 test job that includes analyze-serializables > to the PR checks > 3) create a tailored subset of JDK8-specific tests to include in PR checks > 4) just let the main pipeline catch analyze-serializables failures? > 5) bite the bullet and fix analyze-serializables to test serialization in > a more meaningful way that works on both JDK8 and 11 > > -Owen > > > On May 17, 2019, at 1:21 PM, Darrel Schneider <dschnei...@pivotal.io> > wrote: > > > > One problem with doing this, I think, is that we have some tests that are > > disabled unless run on 8. They are the analyze serializables tests. I'm > not > > sure of the details but I think the "golden" checksums these tests > compare > > to were generated from the byte codes from the jdk 8 class files. The > byte > > codes are different for jdk 11. So by pull requests runs only happening > on > > jdk 11 we will lose coverage. These tests catch if changed the > serializable > > format of classes. I think if the "golden" checksums were regenerated > with > > jdk 11 then these tests could be enabled when run on jdk 11. Others on > the > > dev list may have more of the details. > > > > On Thu, May 16, 2019 at 5:31 PM Owen Nichols <onich...@pivotal.io> > wrote: > > > >> I’ve created a PR for this, please give it a +1: > >> https://github.com/apache/geode/pull/3598 > >> > >>> On May 16, 2019, at 11:12 AM, Anilkumar Gingade <aging...@pivotal.io> > >> wrote: > >>> > >>> Make sense to me...Looking at the probability of breaking specific to, > >> jdk8 > >>> and jdk11 through a commit. > >>> > >>> > >>> On Wed, May 15, 2019 at 6:09 PM Owen Nichols <onich...@pivotal.io> > >> wrote: > >>> > >>>> Currently every PR commit triggers both JDK8 and JDK11 versions of > each > >>>> test job. I propose that we can eliminate the JDK8 version of each > >> check. > >>>> In the extremely rare case where a code change breaks on Java 8 but > >> works > >>>> fine on Java 11, it would still be caught by the main pipeline (just > as > >>>> Windows failures are caught only in the main pipeline). > >>>> > >>>> The only tangible effect today of running both JDK8 and JDK11 tests in > >> PR > >>>> pipeline is twice the chance to encounter possible flaky failures > >> (usually > >>>> unrelated to the commit itself). > >>>> > >>>> > >>>> > >> > >> > >
