RFR(S): 8181592: [TESTBUG] Docker test utils and docker jdk basic test

mikhailo mikhailo.seledtsov at oracle.com
Thu Oct 5 00:27:46 UTC 2017 

Hi Igor,

   Thank you for review. Please see my comments in line.


On 10/03/2017 04:29 PM, Igor Ignatyev wrote:
> Hi Misha,
>
> 1st of all, thank you for working on this very important piece. it 
> generally looks great, although I have a number of mostly stylistic 
> comments.
>
> http://cr.openjdk.java.net/~mseledtsov/8181592.02/test/hotspot/jtreg/runtime/containers/docker/DockerBasicTest.java.html 
> <http://cr.openjdk.java.net/%7Emseledtsov/8181592.02/test/hotspot/jtreg/runtime/containers/docker/DockerBasicTest.java.html>
>>    29  * @requires (docker.support == "true")
> 1. although all properties in requires context are strings, jtreg does 
> support boolean, so this line can be simplified to '@requires 
> (docker.support'
Fixed
>>    31  * @modules java.base/jdk.internal.misc
>>    32  * @modules java.management
>>    33  *          jdk.jartool/sun.tools.jar
> 2. could you please use one style? either add @modules to all lines or 
> leave it only at L#31
Fixed
>
>>    35  * @run main DockerBasicTest
> 3. do we have to run DockerBasicTest in JDK under test? can't it be 
> run as @run driver?
Fixed
>>    46         if (!DockerTestUtils.canTestDocker())
>>    47             return;
> 4. it's been proven to be less error prone to have { } even for 
> one-line blocks, could you please add them here?
OK, updated the changes. Will also use this style from now on.
>
>>    72             .addDockerOpts("--volume", System.getProperty("test.classes") + ":/test-classes/");
> 5. as you already depend on jdk.test.lib, it might be better to 
> reuse Utils::TEST_CLASSES
Fixed

>>    74         DockerTestUtils.dockerRunJava(opts).
>>    75             shouldHaveExitValue(0).shouldContain("Hello Docker");
> 6. lines shouldn't end w/ '.', it should be moved to next line and 
> preferable be alined w/ 1st '.' in prev line. I have seen in other 
> files as well.
> I'd also like to see exact one method call per line.
OK. Updated this and other similar lines to have one statement per line.
>
>
> http://cr.openjdk.java.net/~mseledtsov/8181592.02/test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java.html 
> <http://cr.openjdk.java.net/%7Emseledtsov/8181592.02/test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java.html>
>>    47     // Diagnostics: set to true to enable more diagnostic info
>>    48     private static final boolean DEBUG = false;
> 1. I'd prefer to always have all available diagnostic info. is there 
> any problem w/ having DEBUG=true?
OK, I have removed the DEBUG, and make sure I always output the 
diagnostic info that was conditional on this DEBUG flag.
This also lead to removal of "printChildOutput" parameter for execute(), 
which further simplified the code.
>>   129         Files.createDirectories(jdkDstDir);
>> ...
>>   133         Files.walkFileTree(jdkSrcDir, new CopyFileVisitor(jdkSrcDir, jdkDstDir));
> 2. since jdkDstDir is supposed to be empty, you can use Files::copy 
> method to copy the dir.
Looks like files.copy does not copy the full tree recursively, so I 
still need a CopyFileVisitor.
>
>>   126         Path jdkSrcDir = Paths.get(System.getProperty("test.jdk"));
> 3. again, as you already depend on jdk.test.lib, it might be better to 
> reuse Utils::TEST_JDK
Fixed
>
>>   149     public static void
>>   150         buildDockerImage(String imageName, Path dockerfile, Path buildDir) throws Exception {
>>   213     public static OutputAnalyzer execute(boolean retainChildStdout, String... command)
>>   214         throws Exception {
>>   230     public static OutputAnalyzer execute(boolean retainChildStdout, List<String> command)
>>   231         throws Exception {
> 4. when you wrap a line, you should use double indentation, otherwise 
> one can misread it, in last two examples I read throws Exception as 
> 1st statement of a method.
Fixed
>
> 5. your 'execute(boolean, String...)' creates a list from 'command' 
> array and calls 'execute(boolean, List<String>)', which create an 
> array from 'command' list and pass it to ProcessBuilder::new. should 
> we make 'execute(boolean, List<String>)' call 'execute(boolean, 
> String...)' and move all the logic to the latter?
>
> also, there are easier ways to get a list from an arrays -- 
> j.u.Arrays.asList(T...) or j.u.List.of(T...)
>
Fixed.

I have also corrected/updated JavaDoc comments.

I will post the updated webrev once I address comments from Leonid.


Thank you,
Misha

> Thanks,
> -- Igor
>
>
>> On Sep 28, 2017, at 3:11 PM, Mikhailo Seledtsov 
>> <mikhailo.seledtsov at oracle.com 
>> <mailto:mikhailo.seledtsov at oracle.com>> wrote:
>>
>> Here is the updated webrev:
>> http://cr.openjdk.java.net/~mseledtsov/8181592.02/ 
>> <http://cr.openjdk.java.net/%7Emseledtsov/8181592.02/>
>>
>>
>> Leonid, George - thank you for your comments.
>> In addition to addressing your feedback, I also did:
>>  - implemented @requires docker.support
>>  - added dockerRunJava() method and associated data structure 
>> DockerRunOptions, for running Java tests inside
>>     docker environment, and to account for java opts, test java opts, 
>> docker opts, classes and class params
>>  - added a simple HelloWorld test case that runs HelloWorld inside a 
>> container
>>  - ran jtreg with extra Java options, make sure they are added 
>> correctly to the docker run command
>>  - added docker image cleanup after testing is done
>>
>>
>> Please review.
>>
>> Misha
>>
>>
>> On 9/27/17, 11:00 AM, mikhailo wrote:
>>> Leonid,
>>>
>>> Thank you for review and constructive feedback. See my comment in line.
>>>
>>>
>>> On 09/26/2017 11:19 AM, Leonid Mesnik wrote:
>>>> Misha
>>>>
>>>> http://cr.openjdk.java.net/~mseledtsov/8181592.00/test/hotspot/jtreg/runtime/containers/docker/DockerBasicTest.java.html 
>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8181592.00/test/hotspot/jtreg/runtime/containers/docker/DockerBasicTest.java.html> 
>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8181592.00/test/hotspot/jtreg/runtime/containers/docker/DockerBasicTest.java.html> 
>>>>
>>>> Copyright is incorrect, need to updated it for GPL.
>>> Fixed
>>>>
>>>> The Hotspot is Oracle VM name only so test might fail for OpenJDK. 
>>>> I think you need to fix this check.
>>> I see. I fixed this by using Platform.vmName which should be correct 
>>> in all cases. I double-checked with OpenJDK also.
>>>>
>>>> The requires checks only that test is executed only on the 64-bit 
>>>> linux. Does it make a sense to introduce more docker-specific check?
>>> I agree this is a better way. I will do some prototyping; if such 
>>> check is feasible and efficient in at requires then I will add it.
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~mseledtsov/8181592.00/test/hotspot/jtreg/runtime/containers/docker/Dockerfile-BasicTest.html 
>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8181592.00/test/hotspot/jtreg/runtime/containers/docker/Dockerfile-BasicTest.html> 
>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8181592.00/test/hotspot/jtreg/runtime/containers/docker/Dockerfile-BasicTest.html> 
>>>>
>>>> Could you please explain why oraclelinux 7.0 is used as a base 
>>>> image for test.
>>> I have upgraded to Oracle Linux 7.2. If we have specific requirement 
>>> I will change it to that. If we have requirements in the future to 
>>> support multiple OS, I can add Dockerfile generation.
>>> For this basic sanity tests I think this should suffice.
>>>>
>>>> http://cr.openjdk.java.net/~mseledtsov/8181592.00/test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java.html 
>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8181592.00/test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java.html> 
>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8181592.00/test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java.html> 
>>>>
>>>> The content looks fine.
>>>>
>>>> I don’t see anything to clean up docker images on the system. Could 
>>>> you please explain how tests are going to cleanup images.
>>> To clean up containers I will add "--rm" to the 'docker run' 
>>> command. This should ensure that container data is removed after 
>>> container stops.
>>> As for the image - I use the same image name. The image will stay in 
>>> the local registry unless manually removed. I should probably do 
>>> 'docker rmi' at the end of the test to clean this up.
>>>
>>>
>>> Once I implement these changes I will send the updated webrev.
>>>
>>> Thank you,
>>> Misha
>>>>
>>>> Leonid
>>>>
>>>>
>>>>> On Sep 21, 2017, at 5:58 PM, mikhailo 
>>>>> <mikhailo.seledtsov at oracle.com 
>>>>> <mailto:mikhailo.seledtsov at oracle.com> 
>>>>> <mailto:mikhailo.seledtsov at oracle.com>> wrote:
>>>>>
>>>>> Please review this initial drop of Docker test utils and a sanity 
>>>>> test. This change lays ground
>>>>> for further test development and test utils improvement in this area.
>>>>>
>>>>>    JBS: https://bugs.openjdk.java.net/browse/JDK-8181592
>>>>>    Webrev: http://cr.openjdk.java.net/~mseledtsov/8181592.00/ 
>>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8181592.00/> 
>>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8181592.00/>
>>>>>    Testing:
>>>>>       - run this test on machine with Docker enabled - works
>>>>>       - run this test on Linux-x64 with no Docker engine or Docker 
>>>>> disabled - test skipped (as expected)
>>>>>       - run this test on automated system - in progress
>>>>>
>>>>>
>>>>> Thank you,
>>>>> Misha
>>>>>
>>>>
>>>
>

More information about the hotspot-runtime-dev mailing list