RFR(S): 8164039: Convert test_memset_with_concurrent_readers to GTest
kirill.zhaldybin at oracle.com
Fri Aug 19 17:00:17 UTC 2016
Thank you for reviewing the fix!
Here are a new WebRev:
1. TEST_VM and print_cr were changed to TEST and printf
2. I kept EXPECTs so the test could check all 3 boolean flags but also
ASSERT_TRUE(head_clear && middle_set && tail_clear);
after them which stops the test execution if any of flags are false and
prevents huge amount of not so needed output.
Could you please let me know you opinion?
On 19.08.2016 00:48, Kim Barrett wrote:
>> On Aug 16, 2016, at 1:44 PM, Kirill Zhaldybin <kirill.zhaldybin at oracle.com> wrote:
>> Thank you for reviewing the fix!
>> I changed ASSERTs to EXPECTs.
>> Here are a new webrev: http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8164039/webrev.01/
>> Regards, Kirill
>> On 16.08.2016 20:18, Igor Ignatyev wrote:
>>> Hi Kirill,
>>> I think it’s better to use EXPECT_* instead of ASSERT_* in this test, so we will perform all checks in one run.
>>> otherwise the change looks good to me.
> Using EXPECT rather than ASSERT is very nearly equivalent to just
> removing the asserts in the original code. The existing code already
> provides most of the information and a lot more. The asserts were
> added during development of the original test (earlier they were just
> part of the output, with a failure indication at the very end). The
> test dumps the full block on failure, which makes it easy to see what
> bits are wrong on failure. But if there's one failure, there are
> probably a lot more, and the amount of output generated becomes
> unpleasant to wade through.
> Also, I think this new gtest should be using TEST rather than TEST_VM.
> I assume TEST_VM was used because of the output being generated via
> tty->print_cr. But rather than using that reporting mechanism, the
> new test should be using EXPECT/ASSERT streaming and the like.
More information about the hotspot-gc-dev