RFR(S): 8164028: Convert TestPredictions_test to GTest

Erik Helin erik.helin at oracle.com
Fri Aug 19 10:30:51 UTC 2016


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