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

[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/20150216/e141239d/attachment.htm>