[8u] RFR(s): 8221342: [TESTBUG] Generate Dockerfile for docker testing
Severin Gehwolf
sgehwolf at redhat.com
Mon Aug 31 14:35:13 UTC 2020
Hi Andrew,
On Wed, 2020-08-26 at 04:38 +0100, Andrew Hughes wrote:
> On 15:05 Tue 25 Aug , Severin Gehwolf wrote:
> > Hi!
> >
> > Please review this 8u backport of this test bug which enhances
> > container testing to automatically generate docker files. This is
> > useful if the development platform is not Oracle Linux 7.X (which is
> > the case for me).
> >
> > The JDK 11 patch applies cleanly (after JDK-8220313 which is in the
> > approval queue)
>
> I don't see how this can be said to apply cleanly. Attempting to
> shuffle the patch fails to find a mapping for
> test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java and
> test/lib/jdk/test/lib/containers/docker/DockerfileConfig.java.
>
> It is also beyond the scope of a script to know to duplicate these
> changes between two repositories. So this alone justifies a review.
OK.
> This is beyond the scope of this patch, but the proposed patch
> for the JDK tests seems wrong. We seem to have duplication in the
> JDK test tree, caused by the JFR backport.
Yes, I've noticed it. Question is whether this is worth fixing. Some
tests rely on test/lib/testlibrary/jdk/testlibrary some others
on test/lib/jdk/test/lib.
> The jdk/test/lib/testlibrary/jdk/testlibrary/containers directory
> would be the correct location for these tests. The JFR backport seems
> to have started an alternate tree at jdk/test/lib/jdk/test/lib with
> duplication of some files in the former directory. This should be
> resolved in the 8u282 lifecycle.
I'm not sure I agree but wouldn't object a patch which does this. It
seems a significant piece of work and would require some re-testing of
commonly run tests. Either way, this seems beyond the point of this
patch (as you said).
> > but doesn't compile since Files.writeString() API is
> > not available in JDK 8. Replaced it with:
> >
> > byte[] bytes = dockerFileStr.getBytes(StandardCharsets.UTF_8);
> > Files.write(dockerfile, bytes);
> >
>
> Could this not be simplified to:
>
> Files.write(dockerfile, dockerFileStr.getBytes(StandardCharsets.UTF_8));
>
> I don't see the need for the intermediate variable.
Sure. Fixed:
https://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8221342/jdk8/02/
> Other changes look fine.
Thanks for the review!
Cheers,
Severin
More information about the jdk8u-dev
mailing list