The PR pipeline should be a timely test, but also sane and helpful. Maybe making the JDK8 Unit tests (but not the integration, etc) part of PR is a good compromise in that sense.
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). > >>>> > >>>> > >>>> > >> > >> > >
