@rhoughton - sounds reasonable. JDK8 Unit tests are added back to PR checks now.
@jbarrett - good point, no need to skip analyze-serializables test when compiled with JDK8. Jinmei has a PR to un-skip: https://github.com/apache/geode/pull/3607 @dschneider - do you still want to bring back all JDK8 PR checks for now, or would merging Jinmei’s fix sufficiently address your concerns? > On May 20, 2019, at 8:56 AM, Jacob Barrett <jbarr...@pivotal.io> wrote: > > I that the serialization test was only an issue when compiled with JDK11? We > don’t compile with JDK11, we compile with JDK8 and run under JDK11. > >> On May 20, 2019, at 8:50 AM, Robert Houghton <rhough...@pivotal.io> wrote: >> >> 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). >>>>>>> >>>>>>> >>>>>>> >>>>> >>>>> >>> >>>
