RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

[Mikhailo Seledtsov]

Hi Igor,

   Thank you for reviewing the code.

On 11/6/17, 4:31 PM, Igor Ignatyev wrote:
> Hi Misha,
>
> I have several comments:
> 1. whitebox.cpp : WB_IsContainerized has an incorrect signature, it should be WB_IsContainerized(JNIEnv*, jobject)
Fixed
> 2. CPUSetsReader.java : listToString can be rewritten using stream api as list.stream().limit(maxCount).map(Object::toString).collect(Collectors.joining(","))
Thank you. I will try it out.
> 3. in several files, I have noticed some problems w/ indents which make code quite hard to read, for example
>>    73         Common.run( Common.newOpts(imageName)
>>    74              .addDockerOpts("--memory", valueToSet))
>>    75             .shouldMatch("Memory Limit is:.*" + expectedTraceValue);
> or
>>    96         DockerRunOptions opts = Common.newOpts(imageName, "AttemptOOM")
>>    97             .addDockerOpts("--memory", dockerMemLimit, "--memory-swap", dockerMemLimit);
>>    98             opts.classParams.add("" + sizeToAllocInMb);
OK. I will unwind these expressions, and make sure indentation looks good.
> 4. TestCPUSets.java : it's better to use Assert.assertEquals(parts.length, 2) than Asserts.assertTrue(parts.length == 2) as the former will also print the actual value of parts.length. so you won't need to have L#103
OK
> 5. Test*: there is no need to have parentheses in '@requires (docker.support)'
OK
> 6. Test*: ClassFileInstaller doesn't have to be run by JDK under test, so you can use '@run driver' to run it
Will fix.
> 7. TestCPUSets.java : L#72, to get a proper new line symbol you should use %n in format string. also System.out::printf seems to be more popular in our code than System.out::format
OK.
> 8. TestCPUSets.java: shouldn't checkResult assert that we found lineMarker?
Oops. Missed it. Will add code to check it.
> 9. Test*: would you consider adding .shouldHaveExitValue(0) to all Common.run calls which aren't expected to fail, e.g. all test* in TestCPUAwareness, testMemorySoftLimit and testMemoryLimit in TestMemoryAwareness
Common.run() already checks for .shouldHaveExitValue(0) after running 
the container.


I will apply feedback from Bob and you, and will post a new webrev.


Thank you,
Misha
>
> Thanks,
> -- Igor
>
>> On Nov 1, 2017, at 8:11 PM, mikhailo<mikhailo.seledtsov at oracle.com>  wrote:
>>
>> Please review these tests that were developed to test JVM's container awareness in Docker environment.
>>      JBS: https://bugs.openjdk.java.net/browse/JDK-8189762
>>      Webrev:
>>        Tests: http://cr.openjdk.java.net/~mseledtsov/8189762.00/
>>        WB API: http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/
>>      Testing:
>>          1. Locally: Linux-x64, docker engine version: 17.06.2-ce
>>             Ran the developed tests via jtreg
>>             Pass
>>
>>          2. Automated testing system - run these tests
>>             In progress
>>
>> Thank you,
>> Misha
>>