RFR(S): 8164028: Convert TestPredictions_test to GTest

[Erik Helin]

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.

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

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.

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;

If you read through the hotspot sources you will see that using "const
double" is much more common than "double const".

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.

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.

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
>