RFR(S): 8164028: Convert TestPredictions_test to GTest

[Erik Helin]

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

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