RFR(S): 8164028: Convert TestPredictions_test to GTest
Kirill Zhaldybin
kirill.zhaldybin at oracle.com
Mon Aug 22 13:54:02 UTC 2016
Erik,
Thank you for prompt reply.
Here are a new WebRev:
http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8164028/webrev.04/
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?
fixed
> - there is still an empty line removed in the copyright header for
> test_g1Predictions.cpp
fixed
> - the failure descriptions still says "larger than" instead of
> "greater than"
fixed
> - 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