RFR(S): 8164039: Convert test_memset_with_concurrent_readers to GTest
kirill.zhaldybin at oracle.com
Mon Aug 22 10:42:59 UTC 2016
Here are a new WebRev:
I changed output from printf to stream which then printed by ASSERT.
Could you please let me know your opinion?
On 19.08.2016 23:41, Kim Barrett wrote:
>> On Aug 19, 2016, at 1:00 PM, Kirill Zhaldybin <kirill.zhaldybin at oracle.com> wrote:
>> Thank you for reviewing the fix!
>> Here are a new WebRev: http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8164039/webrev.02/
>> 1. TEST_VM and print_cr were changed to TEST and printf
> I wonder whether printf is appropriate, but I can’t find any indication of where gtest directs its
> output, nor any way to change it. Are you sure it writes to stdout rather than stderr? If the
> latter, or if gtest provides options for directing output somewhere other than stdout, then printf
> isn’t the right function.
>> 2. I kept EXPECTs so the test could check all 3 boolean flags but also added
>> 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?
> Looks good, subject to the above question about using printf.
>> Thank you.
>> Regards, Kirill
>> 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