RFR: 8051984: @ignore should be placed after @test

[Andrey Zakharov]

Do I need another review or review of Bengt is enough?

Thanks.

16.02.2015 22:41, Bengt Rutisson пишет:
>
> Hi again,
>
> On 16/02/15 17:07, Andrey Zakharov wrote:
>>
>> 16.02.2015 18:41, Bengt Rutisson пишет:
>>>
>>> Hi Andrey,
>>>
>>> On 16/02/15 16:24, Andrey Zakharov wrote:
>>>> Hi
>>>>> But JDK-8019361 looks like a wrong number.  It must be JDK-8051984, I think.
>>>>
>>>> I'm wrongly got reason of ignore bug for RFR. Thanks, Dima.
>>>>
>>>> webrev:
>>>> http://cr.openjdk.java.net/~azakharov/8051984/webrev//
>>>>
>>>> /bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8051984
>>>>
>>>>> If so, what about other sources listed in the description:
>>>>>     ./test/gc/arguments/TestParallelHeapSizeFlags.java
>>>>>     ./test/gc/arguments/TestUseCompressedOopsErgo.java
>>>>>     ./test/gc/g1/TestHumongousShrinkHeap.java
>>>> Its already fixed either by removing @ignore either by inserting 
>>>> @requires
>>>> /
>>>> /
>>>>> Why did you change the static import of
>>>>> com.oracle.java.testlibrary.Asserts? Seems unrelated to the @ignore
>>>>> change and I don't think there is a reason for it either. We use static
>>>>> import of the asserts a lot in our test code.
>>>>>
>>>>> Thanks,
>>>>> Bengt
>>>>>
>>>>> >/
>>>>> />/  Thanks./
>>>>
>>>> There is nothing especial in import static here, its  only serves 
>>>> to reduce Asserts package names in code, but it also leads to less 
>>>> readability and question like "what assertLessThan comes from?".
>>>> Asserts.assertLessThan is better - it doesn't junk global 
>>>> namespace. In only this case - its only code style question. If you 
>>>> have any other conserns about this, please tell me.
>>>
>>> For people reading the hotspots tests a lot I think assertLessThan() 
>>> is more readable than Asserts.assertLessThan(). Please don't change 
>>> this for this test only. If you feel strongly about this issue I 
>>> think you should bring it up for a wider discussion so we can agree 
>>> on a guideline for how to use it. Right now it just seems like a 
>>> completely unrelated change that is not necessary to solve your bug.
>> Ok. At least I must change Asserts.* to Asserts.assertLessThan;
>
> I still find it odd to do this as part of this change. But ok.
>
>>
>> webrev:
>> http://cr.openjdk.java.net/~azakharov/8051984/webrev.02/
>
> Looks ok. Reviewed.
>
>>
>> Just for information:
>> in src/hs-gc/hotspot/test
>>
>> grep 'import com.oracle.java.testlibrary.Asserts.' -R ./ 
>> --exclude-dir=.hg | wc -l
>> 56
>>
>> grep 'import static com.oracle.java.testlibrary.Asserts.' -R ./ 
>> --exclude-dir=.hg | wc -l
>> 66
>>
>> grep 'import static com.oracle.java.testlibrary.Asserts.' -R ./gc 
>> --exclude-dir=.hg | wc -l
>> 17
>>
>> grep 'import com.oracle.java.testlibrary.Asserts.' -R ./gc 
>> --exclude-dir=.hg | wc -l
>> 5
>
> Right, so static import is the more common way for importing the asserts.
>
> There are also the JFR tests for hotspot:
>
> $ grep -r 'import static jrockit.Asserts' 
> jdk/test/closed/com/oracle/jfr/gc/ | wc -l
>       36
>
> $ grep -r 'import jrockit.Asserts' jdk/test/closed/com/oracle/jfr/gc/ 
> | wc -l
>        3
>
> Thanks,
> Bengt
>
>>
>> Thanks.
>>
>>>
>>> Thanks,
>>> Bengt
>>>
>>>>
>>>>
>>>> Thanks.
>>>>
>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150226/9058a92a/attachment.htm>