RFR(S): 8164028: Convert TestPredictions_test to GTest

Kirill Zhaldybin kirill.zhaldybin at oracle.com
Mon Aug 22 11:39:14 UTC 2016 

Erik,

Here are a new WebRev: 
http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8164028/webrev.03/
Changes:
1. utilities/macros.hpp deleted
2. Asserts were made coherent with messages.
3. hg move

Could you please let me know your opinion?

Thank you.

Regards, Kirill

On 22.08.2016 11:54, Erik Helin wrote:
> On 2016-08-19, Kirill Zhaldybin wrote:
>> Erik,
>>
>> Here are a new WebRev:
>> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8164028/webrev.02/
>>
>> Changes:
>> 1. Moved epsilon to static const
>> 2. Changed test category and test names
>> 3. Changed ASSERT_LT to ASSERT_NEAR where applicable
>> 4. Deleted redundant output in asserts
>> 5. Moved tests' descriptions to precede tests
>> 6. Reverted extra empty line removal in copyright.
>>
>> Could you please let me know your opinion?
> Looks much better. A couple of comments:
> - do you really need to include utilities/macros.hpp?
> - In the following assert
>    ASSERT_LT(p2, p1) << "First prediction must be larger than second";
>    you use ASSERT_LT (ASSERT_LESS_THAN) and then the comment says "larger
>    than". Could you please use coherent asserts and failure descriptions?
>    For example:
>    ASSERT_GT(p1, p2) << "First prediction must be greater than second";
>    In the above example both the assert and the failure description uses
>    the same wording, "greater than".
> - in order to preserve history, could you do
>    `hg mv src/share/vm/gc/g1/g1Predictions.cpp
>    test/native/gc/g1/test_g1Predictions.cpp`? If the move is done
>    correctly, then you will see in the webrev that the file has been
>    moved.
>
> Thanks,
> Erik
>
>> Thank you.
>>
>> Regards, Kirill
>>
>> On 19.08.2016 13:30, Erik Helin wrote:
>>> On 2016-08-18, Kirill Zhaldybin wrote:
>>>> 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?
>>> I'm suggesting TEST_VM(<class_name>, <test_case_name>), *not*
>>> TEST_VM(<source_file_name>, <test_case_name>). Using category for the
>>> testname has IMO two major drawbacks:
>>> - The complete name of the test must be unique, so if you have to
>>>    different classes, e.g. GrowableArray and Stack, and both had a
>>>    get_value method, you can't write: TEST_VM(utilities, get_value) in
>>>    both test_growableArray.cpp and test_stack.cpp.
>>> - The output from running the tests will be less clear, you will see the
>>>    test name 'gc.basic_predictions_test_vm'. This could mean anything
>>>    from testing GC timing predictions in G1Policy to testing predictions
>>>    in G1Predictions. Seeing G1Predictions.basic_predictions_test_vm makes
>>>    it much more clear what is being tested.
>>>
>>> I agree that filtering the tests is harder, but the idea is to run all
>>> the tests all of the time, they shouldn't take a lot of time to execute.
>>> All the TEST and TEST_VM tests are C++ functions being called inside a
>>> running process, we would need quite a few such tests before they take
>>> too long time. The tests that takes more time are TEST_OTHER_VM since
>>> they spawn a new process, but those tests can already be filtered out
>>> since they always end in _test_other_vm.
>>>
>>> If we one day have so many TEST and TEST_VM tests that running them
>>> takes too long, then I probably would prefer to enhance the macros and
>>> for example have TEST_VM_GC that prepends gc_ to the test name. I will
>>> also be very happy that we then have A LOT of unit tests :)
>>>
>>>>>> 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.
>>> Please don't mix any eventual fixes of the formatting of the copyright
>>> header with actual changes to the source code. Let Alexander fix all of
>>> the copyright formatting problems is his patch and let this patch focus
>>> on the source code changes.
>>>
>>> Thanks,
>>> Erik
>>>
>>>>>> 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

More information about the hotspot-gc-dev mailing list