RFR(S): 8164028: Convert TestPredictions_test to GTest

Kirill Zhaldybin kirill.zhaldybin at oracle.com
Mon Aug 22 13:54:02 UTC 2016


Thank you for prompt reply.

Here are a new WebRev: 

Could you please read comments inline?

Thank you.

Regards, Kirill

On 22.08.2016 15:09, Erik Helin wrote:
> On 2016-08-22, Kirill Zhaldybin wrote:
>> 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
> A couple of comments:
> - what happened to the change to g1Predictions.hpp?
> - there is still an empty line removed in the copyright header for
>    test_g1Predictions.cpp
> - the failure descriptions still says "larger than" instead of
>    "greater than"
> - Looking at the unified diff (much easier now, thanks) it seems like
>    the old code used <, which means that the new code must use ASSERT_GE,
>    not ASSERT_GT.
Sorry, I did not find a line you mentioned. Could you please let me know 
the line number?
> Otherwise it looks good.
> Thanks,
> Erik
>> 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