RFR(S): 8164028: Convert TestPredictions_test to GTest
Erik Helin
erik.helin at oracle.com
Mon Aug 22 08:54:46 UTC 2016
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