RFR(S): 8164028: Convert TestPredictions_test to GTest

Erik Helin erik.helin at oracle.com
Mon Aug 22 14:03:26 UTC 2016 

On 2016-08-22, Kirill Zhaldybin wrote:
> 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?

Just look at the unified diff:
http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8164028/webrev.04/test/native/gc/g1/test_g1Predictions.cpp.udiff.html

Everywhere you see a pattern similar to:
-  assert(p2 < p1, "First prediction must be larger than second, but they are %f %f", p1, p2);
+  ASSERT_GT(p1, p2) << "First prediction must be greater than second";

you need to change ASSERT_GT to ASSERT_GE.

Thanks,
Eirk

> >
> >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