<AWT Dev> Review request for JDK-8039279: Move first batch of functional tests to openjdk repository

[Petr Pchelko]

Hello,

>> import static sun.awt.OSInfo.*;
>> 
>> Why is this using sun.* API ?
>> In module mode this will be invisible.
>> 
>> And all its used for is this :-
>> if (getOSType() == OSType.WINDOWS)
>> 
>> You can do that with a getProperty() call.
> 
> Personally, I agree with that and try not to use OSInfo.
> However the author of the fix was forced to use it by
> previous reviewers. It is considered a standard routine
> by now. 
Right now the tests are filled with this pattern... And with other
private APIs. We have SunToolkit in almost every single test, but it's
really better to not use private APIs in new tests.
We didn't think about Jigsaw when making this comment,
probably using the os.name property would be better.
Sorry, Dmitriy, look like I've pushed you to the wrong path here.

>>   36  * @build ExtendedRobot jdk.testlibrary.Asserts
>> 
>> What is ExtendedRobot ? I can't find it.
>> 
>> 
>> And do we really need a library dependency just to
>> do equals for us ??
> Re: ExtendedRobot please see
> https://bugs.openjdk.java.net/browse/JDK-8038631
> and explanations of our objectives there.
> It was discussed in this list and run through 7 cycles
> of review.
> In short, we have hundreds of existing tests depending
> on very complex Robot extensions. We have to move all
> of them to the regression suite. And exactly to get rid of
> (even) binary dependencies we proposed a minimal extension
> which is both useful and allows us not to rewrite the bulk
> of old code whenever possible.
> This portion of tests also is not written anew
> (which would be easier, it seems) but ported from functional
> suite.
Just to add, we expect that the ExtendedRobot features will be
moved to the awt Robot and made public API when the process 
of test migration moves on and the features would be baked in 
the ExtendedRobot. Additionally we will try to import all of our
other helpers into Robot at some point.

>>   73             assertEquals(bound, max,
>>   74                     "The bounds of the Frame equal to what"
>>   75                     + " is specified when the frame is in
>> Frame.MAXIMIZED_BOTH state");
>> 
>> Could easily be replaced by
>> if (!bounds.equals(max)) throw new RuntimeException(...);
>> 
>> We should make the tests as easy to run standalone as possible
>> and avoid internal APIs.
> 
> Hear, hear!
> Other colleagues here have different opinions though.
This was also my idea to use this. The code looks cleaner with it,
but it's probably the matter of taste. Currently we use awt and swing 
regtesthelpers, process helpers and JRobot in almost every test,
so I thought it wouldn't be harmful to use yet another library. No strong
opinion here, so if Phil thinks this practice is bad I'd agree with him.

With best regards. Petr.

On 16.04.2014, at 10:41, Yuri Nesterenko <yuri.nesterenko at oracle.com> wrote:

> On 04/15/2014 10:26 PM, Phil Race wrote:
>> http://cr.openjdk.java.net/~yan/8039279/webrev.02/test/java/awt/Frame/SetMaximizedBounds/SetMaximizedBounds.java.html
>> 
>> 
>> 
>> import static sun.awt.OSInfo.*;
>> 
>> Why is this using sun.* API ?
>> In module mode this will be invisible.
>> 
>> And all its used for is this :-
>> if (getOSType() == OSType.WINDOWS)
>> 
>> You can do that with a getProperty() call.
> 
> Personally, I agree with that and try not to use OSInfo.
> However the author of the fix was forced to use it by
> previous reviewers. It is considered a standard routine
> by now. For jigsaw, we'll have to invent something
> to accommodate that.
> 
>> 
>> 
>>   36  * @build ExtendedRobot jdk.testlibrary.Asserts
>> 
>> What is ExtendedRobot ? I can't find it.
>> 
>> 
>> And do we really need a library dependency just to
>> do equals for us ??
> Re: ExtendedRobot please see
> https://bugs.openjdk.java.net/browse/JDK-8038631
> and explanations of our objectives there.
> It was discussed in this list and run through 7 cycles
> of review.
> In short, we have hundreds of existing tests depending
> on very complex Robot extensions. We have to move all
> of them to the regression suite. And exactly to get rid of
> (even) binary dependencies we proposed a minimal extension
> which is both useful and allows us not to rewrite the bulk
> of old code whenever possible.
> This portion of tests also is not written anew
> (which would be easier, it seems) but ported from functional
> suite.
> 
>> 
>>   73             assertEquals(bound, max,
>>   74                     "The bounds of the Frame equal to what"
>>   75                     + " is specified when the frame is in
>> Frame.MAXIMIZED_BOTH state");
>> 
>> Could easily be replaced by
>> if (!bounds.equals(max)) throw new RuntimeException(...);
>> 
>> We should make the tests as easy to run standalone as possible
>> and avoid internal APIs.
> 
> Hear, hear!
> Other colleagues here have different opinions though.
> 
> -yan
> 
>> 
>> -phil
>> 
>> On 4/15/2014 6:14 AM, Sergey Bylokhov wrote:
>>> Hi, Dmitriy.
>>> A few comments:
>>> SetMaximizedBounds.java:  please add macosx to the test coverage. It
>>> does not work now but the fix is under review.
>>> ChangeGridSize.java/ ComponentPreferredSize.java: actionPerformed must
>>> be volatile, all other constant can be final.
>>> ModifierRobotKeyTest.java: tempPress must be volatile, access to
>>> modifierStatus[] and textStatus should be synchronized somehow.
>>> LockingKeyStateTest.java:I guess we have synchronization problem as well.
>>> 
>>> Additionally, can you dispose all frames in the tests.
>>> 
>>> On 4/15/14 3:33 PM, Dmitriy Ermashov wrote:
>>>> Hi all.
>>>> 
>>>> Petr, thanks for review!
>>>> 
>>>> Guys, could you also review this tests?
>>>> http://cr.openjdk.java.net/~yan/8039279/webrev.02/
>>>> 
>>> 
>>> 
>>> --
>>> Best regards, Sergey.
>> 
>