On 04/16/2014 12:32 PM, Petr Pchelko wrote:
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.
I agree with Phil that we have no real need in usage Asserts library here.
But I suppose that most of tests that uses this dependency can do
without it. And it is not the reason to rewrite tests.
Asserts usage looks more clear and that's why it makes sense.
I believe that elegance and readability must be over independence. And
this rule touches other functional tests.
With best regards. Petr.
On 16.04.2014, at 10:41, Yuri Nesterenko <[email protected]> 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.
--
Thanks,
Dima
