RFR(S): 8164028: Convert TestPredictions_test to GTest

[Kirill Zhaldybin]

Erik,

Could you please read comments inline?

Thank you.

Regards, Kirill

On 18.08.2016 12:14, Erik Helin wrote:
> On 2016-08-18, Erik Helin wrote:
>> On 2016-08-17, Kirill Zhaldybin wrote:
>>> Erik,
>>>
>>> Thank you for reviewing the fix!
>>> I changed test names to snake_case.
>>>
>>> Here are a new WebRev:
>>> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8164028/webrev.01/
>>
>> Sorry, I should have provided more comments on the initial patch. Given
>> that you are testing the class G1Predictions, the category should
>> G1Predictions, not. I.e. the first test should be
>>
>>     TEST_VM(G1Predictions, basic_predictions)
>>
>> As you can see above, there is no need to have 'test' anywhere in the
>> name of the test, since the above macro will expand to:
>>
>>     TEST(G1Predictions, basic_predictions_test_vm)
>>
>> Please use this naming convention.
According to 
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#running-a-subset-of-the-tests 
the way to run a subset of tests is to specify filter string.

Naming tests in TEST_VM(gc, predictions_basic) way allows us to run all 
gc-related tests with simple filter like "gc.*".

If we use TEST_VM(<source_file_name>, <test_case_name>) we should create 
pretty long filter strings if we want to run test from specific area 
(gc, runtime, compiler etc).
Could you please let me know your opinion?


>>
>> Furthermore, there is no need to write lengthy descriptions for each
>> assert, like in:
>>
>>    42   ASSERT_LT(fabs(p1 - 5.0), epsilon) << "Prediction should be 5.0
>>    but is " << p1;
>>
>> One benefit of using googletest is that the ASSERT_LT macro will show
>> both values if the assertion fail. The above can simply be reduced to:
>>
>>    42   ASSERT_LT(fabs(p1 - 5.0), epsilon);
>
> Looking at this some more, why use ASSERT_LT? It seems like
> ASSSERT_NEAR(p1, 5.0, epsilon) is a much better fit. See
> https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#floating-point-macros
> for more details.
Thank you for noticing this. I will fix it in next iteration.
>
> Thanks,
> Erik
>
>> There is also no need to include information such as "First prediction
>> ... " in error message since the assert macros provided by googletest
>> will show the file and line number in the assert messages.
>>
>> Finally, if you want to add comments to describe the test, then please
>> put the comments above the test function, as in:
>>
>>    // The following test checks that the initial predictions are based on
>>    // the average of the sequence and not on the stddev (which is 0).
>>    TEST_VM(gc, test_predictions_average_not_stdev) {
>>
>> Having the comments as the second (?) line in the tests are just
>> confusing.
I will fix it in next iteration.
>>
>> Furthemore, since you are using the same epsilon in all the test
>> methods, please put it as a static constant in the test file (the tests
>> are just ordinary C++ code):
>>
>>    static const double epsilon = 1e-6;
I will fix it in next iteration.
>>
>> If you read through the hotspot sources you will see that using "const
>> double" is much more common than "double const".
I will fix it in next iteration.
>>
>> Continuing, you shouldn't need #if INCLUDE_ALL_GCS, the Makefile
>> will not compile hotspot/test/native/gc/g1 when it doesn't compile
>> hotspot/src/share/vm/gc/g1. Please remove #if INCLUDE_ALL_GCS.
I will fix it in next iteration.

>>
>> Finally, you removed a newline from the copyright header in
>> g1Predictions.hpp:
>>
>> @@ -19,7 +19,6 @@
>>    * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
>>    * or visit www.oracle.com if you need additional information or have any
>>    * questions.
>> - *
>>    */
>>
>> Please revert this part of the patch.
According to 
https://wiki.se.oracle.com/display/JPG/JDK+Copyright+Guidelines#JDKCopyrightGuidelines-GPLSources(openjdksources)GPL 
there are no extra newline in copyright notice.
As far as I know Alexander Iline is fixing this in other files.
>>
>> Thanks,
>> Erik
>>
>>> Regards, Kirill
>>>
>>> On 17.08.2016 18:08, Erik Helin wrote:
>>>> On 2016-08-17, Kirill Zhaldybin wrote:
>>>>> Dear all,
>>>>>
>>>>> Could you please review this fix for JDK-8164028?
>>>>>
>>>>> The test was converted to gtrest, a couple of wrong checks fixed.
>>>>>
>>>>> WebRev: http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8164028/webrev.00/
>>>> It seems like most the unit tests in hotspot/test/native uses
>>>> "snake_case" for naming the tests, whereas you are using "camelCase".
>>>> Would you mind changing to "snake_case" so the test names are
>>>> consistent?
>>>>
>>>> Thanks,
>>>> Erik
>>>>
>>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8164028
>>>>>
>>>>> Thank you.
>>>>>
>>>>> Regards, Kirill
>>>