jdk icon indicating copy to clipboard operation
jdk copied to clipboard

Published 20 hours ago • verified publisher icon openjdk

JDK-8290469: Add new positioning options to PassFailJFrame test framework

Open honkar-jdk opened this issue 3 years ago • 17 comments
trafficstars

Additional position setting (TOP_LEFT_CORNER) and a method to obtain bounds of test instruction frame are added to PassFailJFrame to handle positioning of multiple test frames.

In scenarios where multiple test windows might be present, the test windows might overlap the instruction frame. In order to fix this TOP_LEFT_CORNER position option is added that positions the test instruction frame at top left corner with main test window below it.

Additionally getInstructionFrameBounds() is added to obtain the position and dimensions of test instruction frame.

Progress

  • [x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • [x] Change must not contain extraneous whitespace
  • [x] Commit message must refer to an issue

Issue

  • JDK-8290469: Add new positioning options to PassFailJFrame test framework

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9525/head:pull/9525
$ git checkout pull/9525

Update a local copy of the PR:
$ git checkout pull/9525
$ git pull https://git.openjdk.org/jdk pull/9525/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9525

View PR using the GUI difftool:
$ git pr show -t 9525

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9525.diff

honkar-jdk avatar Jul 16 '22 01:07 honkar-jdk

:wave: Welcome back honkar! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

bridgekeeper[bot] avatar Jul 16 '22 01:07 bridgekeeper[bot]

@honkar-jdk The following label will be automatically applied to this pull request:

  • client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

openjdk[bot] avatar Jul 16 '22 01:07 openjdk[bot]

Webrevs

mlbridge[bot] avatar Jul 16 '22 01:07 mlbridge[bot]

Another approach would be to use a JSplitPane to have the instruction frame and the main test frame as one window. Left half/top as instruction frame and Right half/ bottom as the main test window and this also provides HORIZONTAL_SPLIT or VERTICAL_SPLIT orientation equivalent to HORIZONTAL and VERTICAL positioning of PassFailJFrame.

Advantage of this approach:

  • No need to manually position the instruction frame and then the test window, as it will be one window - JSplitPane
  • Entire window (test + instruction frame) need to be positioned only once.
  • With this change, we might no longer need positionTestWindow() method, additionally the framework code will be simple and straightforward.

honkar-jdk avatar Jul 28 '22 17:07 honkar-jdk

I can confirm that on Linux / Gnome with menu bar at the top and toolbar on the left, that the requested location of 0,0 is not where the window is placed but if called immediately both getLocation() and getLocationOnScreen() may report back (0,0) leading to incorrect positioning of the 2nd window.

Calling Toolkit.sync() isn't enough to ensure it is always correct. In two runs of the same test code (my code below but without a sleep call) in one case it then reports (0,0) and in another (74,27) - the actual location.

Calling isVisible() or isShowing() don't help.

The simplest thing that works every time for me is to add a short sleep() to give time for everything to be processed. I didn't need the Robot. It works both off and on the EDT. Not 100% satisfactory as there really ought to be a better way but it seems much more reliable than what we had and it keeps it easy to use. I only tried this on Ubuntu 22.04 .. you'll need to test some other platforms So just give that a try but do KEEP the insets calculations for the safest results. import java.awt.*;

public class F {

public static void main(String[] args) throws Exception { //doit(); EventQueue.invokeAndWait(F::doit); }

public static void doit() {

Frame f1 = new Frame("Frame 1");
f1.setSize(300,200);
f1.setLocation(0,0);
f1.show();

Toolkit.getDefaultToolkit().sync();
try { Thread.sleep(500); } catch (InterruptedException e) {}
System.out.println("screen location ="+f1.getLocationOnScreen());
System.out.println("frame location ="+f1.getLocation());
System.out.println("frame size ="+f1.getSize());

int f2x = f1.getLocation().x+f1.getWidth();
int f2y = f1.getY();
System.out.println("desired frame 2 location = " + f2x + ", "+ f2y);
Frame f2 = new Frame("Frame 2");
f2.setSize(200,300);
f2.setLocation(f2x, f2y);
f2.show();
System.out.println("frame 2 location = " + f2.getLocation());

} }

prrace avatar Jul 29 '22 20:07 prrace

I can confirm that on Linux / Gnome with menu bar at the top and toolbar on the left, that the requested location of 0,0 is not where the window is placed but if called immediately both getLocation()

@prrace Thank you for checking it on Linux. Adding a delay would be the best option to get the updated location values in this scenario then.

Another approach that I had in mind was to change PassFailJFrame to use a JSplitPane (single window) for instructions and test window. This way there is no hassle of managing the positioning of 2 windows as described above https://github.com/openjdk/jdk/pull/9525#issuecomment-1198453250

Please let me know your suggestions on JSplitPane approach.

honkar-jdk avatar Jul 29 '22 21:07 honkar-jdk

Please let me know your suggestions on JSplitPane approach.

I think it very unlikely a JSplitPane would satisfy all test requirements.

prrace avatar Jul 29 '22 21:07 prrace

Please let me know your suggestions on JSplitPane approach.

I think it very unlikely a JSplitPane would satisfy all test requirements.

@prrace Do you mean - a multiple frame test scenario or any other scenario in particular? We could readjust the splits (divider location) based on different test requirements and make the test window optional (if required). Currently for the multiple frame scenario - we have a instruction window and a root test frame , the rest of the frames are arranged at test-level.

honkar-jdk avatar Jul 29 '22 21:07 honkar-jdk

Well .. there surely must be test scenarios where a Frame is required. Perhaps the test moves it, iconifies, needs a specific size .. expects focus to move in a certain order between components in the frame .. wants to use heavyweight AWT components .. .in some of these I expect the extra instruction part doesn't matter but you only have to find ONE case where it matters .. then there's the fact you'd have to rewrite all the existing tests. And why a JSplitPane, anyway ? Odd choice.

I can imagine that it might be interesting to add a new version that works with a JPanel as the container for the test and let a test author decide if they want to use that for future tests.

prrace avatar Jul 29 '22 22:07 prrace

@prrace I see your point and the broader range of testing requirements that might need to be supported by the test framework in future such as iconifying/ minimizing test frames.

Earlier when I suggested JSplitPane, I was looking it from ease of positioning, disposing and that most of the manual tests had more or less - one instruction frame and one test window which could be achieved using the JSplitPane. I probably missed analyzing the part about broader support of framework for future test cases.

Another approach would be to use a JSplitPane to have the instruction frame and the main test frame as one window. Left half/top as instruction frame and Right half/ bottom as the main test window and this also provides HORIZONTAL_SPLIT or VERTICAL_SPLIT orientation equivalent to HORIZONTAL and VERTICAL positioning of PassFailJFrame.

Advantage of this approach:

  • No need to manually position the instruction frame and then the test window, as it will be one window - JSplitPane
  • Entire window (test + instruction frame) need to be positioned only once.
  • With this change, we might no longer need positionTestWindow() method, additionally the framework code will be simple and straightforward.

honkar-jdk avatar Jul 29 '22 23:07 honkar-jdk

I don't think JSplitPane is a bad idea, rather the opposite: it's a good idea. It encapsulates a JPanel as a container for test UI. If a test doesn't add UI to that area (via a callback, or lambda or method reference, or any other way), the instructions UI could be left as the sole content of the frame. Otherwise, JSplitPane will separate the parts in the frame and will allow changing the size of the panes. It also mimics the original positioning strategy: vertical/horizontal.

In some tests, the test UI consists of a single button only. I think a single window for such test UI would work better than two separate and independent frames. One frame is easier to manage.

I was thinking about merging instructions and test UI from the start. Yet using two frames / windows seemed easier to implement.

Some test cases may not fit into either pattern. There's always the option to create test UI as needed. Yet positioning of the instruction frame may be an issue. If test creates windows / dialogs owned by the instruction frame, AWT ensures the test UI windows / dialogs are above the instruction frame; however, if test UI is large enough, it could completely overlap the instructions.

I guess some test cases may even be easier to implement and maintain with its own custom UI. Too many options in PassFailJFrame could make it harder to use.

aivanov-jdk avatar Aug 02 '22 20:08 aivanov-jdk

After evaluating possible alternative solutions as well as suggestions provided by @prrace & @aivanov-jdk, I think introducing small delay (Thread.sleep) would be the best option - for syncing the updated position to window manager and keeping in mind the broader range of testing requirement that the framework needs to support for now.

honkar-jdk avatar Aug 03 '22 19:08 honkar-jdk

The recent commit has the following changes -

  • Toolkit sync and Thread.sleep added to push the frame's updated location to window manager
  • Position of setVisible for instruction frame moved from createUI to positionTestWindow. Before this change the frame was initially visible for sometime in its initial location for a few seconds before moving to its final position set in positionTestWindow.
  • For the same reason as above, the setVisible for testWindow is called after positionTestWindow() at test-level. The switch from the initial to the final position is more evident due to Thread.sleep hence the position of setVisible is changed in tests and PassFailJFrame.

honkar-jdk avatar Aug 05 '22 00:08 honkar-jdk

Added null check for testWindow in positionWindow as the following line testWindow.setLocation(.....); will produce NPE if positionWindow is called with testWindow = null.

In some tests testWindow is not required, for example TrayIconScalingTest.java in which case we can call PassFailJFrame.positionTestWindow(null,PassFailJFrame.Position.HORIZONTAL);

honkar-jdk avatar Aug 05 '22 17:08 honkar-jdk

@honkar-jdk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8290469: Add new positioning options to PassFailJFrame test framework

Reviewed-by: prr, aivanov

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 32 new commits pushed to the master branch:

  • 69448f9c1a9b123be8ef63bb039111a8976a8da6: 8292679: Simplify thread creation in gtest and port 2 tests to new way
  • 3c2289d65157fca3f303d2fe703b31ef53b2f5bf: 8215916: The failure reason of an optional JAAS LoginModule is not logged
  • 71ab5c95af28497fb31aba8ba9597da71bc4d3d5: 8292816: GPL Classpath exception missing from assemblyprefix.h
  • c0623972cffdbcd7f80e84a1ec344fd382a4a5cc: 8292713: Unsafe.allocateInstance should be intrinsified without UseUnalignedAccesses
  • a45a4b9465d7d01715425000c4fd47c0aa3e76ca: 8292194: G1 nmethod entry barrier disarm value wraps around too early
  • d3fed128671312ae138a7ad5677c3c140de9a8f3: 8292864: Remove resourceHash.hpp from header files
  • b653b9cc274abb906d3ebe887827ef16b4e57be4: 8291969: Convert LoaderConstraintsTable to ResourceHashtable
  • 0813a4705184e33f6bd175dcd9a17d3212890daf: 8282729: Serial: Move BOT implementation to collector specific directory
  • fa5cc4cc8e1c7c1f627905191e311cfcb3d94b5e: 8291878: NMT: Malloc limits
  • ad2e0c4df045261c04b00bfa1faf5c21392edc58: 8292778: EncodingSupport_md.c convertUtf8ToPlatformString wrong placing of free
  • ... and 22 more: https://git.openjdk.org/jdk/compare/aa9b8f04bf74d5fa00f2b27895e7369abea3a930...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@prrace, @aivanov-jdk) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk[bot] avatar Aug 12 '22 13:08 openjdk[bot]

@prrace PR has been updated with suggested changes and ready for re-review.

honkar-jdk avatar Aug 15 '22 18:08 honkar-jdk

I can imagine that it might be interesting to add a new version that works with a JPanel as the container for the test and let a test author decide if they want to use that for future tests.

As suggested, further changes to PassFailJFrame (creating a new version) will be dealt as a separate issue after evaluating impact of redesigning the framework.

honkar-jdk avatar Aug 15 '22 18:08 honkar-jdk

As suggested, further changes to PassFailJFrame (creating a new version) will be dealt as a separate issue after evaluating impact of redesigning the framework.

This will be taken up as a separate task and does not affect the present PR. Following are proposed redesign changes to PassFailJFrame test framework.

  1. Addition of JSplitPane – for instruction and test window
  2. If an exception is thrown before awaitAndCheck is called, the test will hang eventually. Clicking the Pass/Fail buttons doesn't close the UI since it's inside awaitAndCheck. This needs to be addressed when redesigning the framework.
  3. Functional Style Paradigm: so that the code to create the test UI (window or component) can be passed as a method reference to constructor.
  4. Handler for the close button on test UI windows / frames (obtained via a factory method which would fail the test and dispose the UI). Currently it's to be handled by each test (usually remain unhandled).
  5. Better formatting option for instruction window – support for html tags.

The plan is to start the changes as a new and separate test framework to minimize the impact on existing manual test cases using PassFailJFrame. If anyone has anything additional, please feel free to add or provide feedback on this discussion.

honkar-jdk avatar Aug 18 '22 22:08 honkar-jdk

Adding a button 'Screen capture' ( Screen shot ) in test instruction frame when user clicks to fail button to capture the failure scenario will be good idea. Screen shot to capture both test instruction frame as well as Test frame. This will help every one to know the following

  1. Test scenario that user is testing
  2. What exactly user is seeing on the screen ( test frame ) along with failure reason that user is entering now.

lawrence-andrew avatar Aug 22 '22 21:08 lawrence-andrew

Adding a button 'Screen capture' ( Screen shot ) in test instruction frame when user clicks to fail button to capture the failure scenario will be good idea. Screen shot to capture both test instruction frame as well as Test frame. This will help every one to know the following

  1. Test scenario that user is testing
  2. What exactly user is seeing on the screen ( test frame ) along with failure reason that user is entering now.

Another thing do later in a follow on RFE. It is a stretch to add that to a fix that adds a new positioning option.

prrace avatar Aug 22 '22 21:08 prrace

Based on this discussion https://github.com/openjdk/jdk/pull/9525#discussion_r951837454, Added the term "approximately positions" to positionTestWindow() javadoc.

honkar-jdk avatar Aug 22 '22 22:08 honkar-jdk

/integrate

honkar-jdk avatar Aug 22 '22 22:08 honkar-jdk

@honkar-jdk Your change (at version 2c72bc3508f2527848ae3428d5786660fd8e513b) is now ready to be sponsored by a Committer.

openjdk[bot] avatar Aug 22 '22 22:08 openjdk[bot]

Adding a button 'Screen capture' ( Screen shot ) in test instruction frame when user clicks to fail button to capture the failure scenario will be good idea. Screen shot to capture both test instruction frame as well as Test frame. This will help every one to know the following

  1. Test scenario that user is testing
  2. What exactly user is seeing on the screen ( test frame ) along with failure reason that user is entering now.

Another thing do later in a follow on RFE. It is a stretch to add that to a fix that adds a new positioning option.

The above suggestion was meant to be added by @lawrence-andrew as part of the test framework redesign discussion https://github.com/openjdk/jdk/pull/9525#issuecomment-1220057640

honkar-jdk avatar Aug 22 '22 22:08 honkar-jdk

Adding a button 'Screen capture' ( Screen shot ) in test instruction frame when user clicks to fail button to capture the failure scenario will be good idea. Screen shot to capture both test instruction frame as well as Test frame.

Capturing the entire screen is a better option. The test may create other windows, the image of the entire screen would be more useful.

aivanov-jdk avatar Aug 23 '22 18:08 aivanov-jdk

/integrate

honkar-jdk avatar Aug 24 '22 16:08 honkar-jdk

@honkar-jdk Your change (at version 2db5b8a3453bdafd0bd99d56f78af46dd0ac67b7) is now ready to be sponsored by a Committer.

openjdk[bot] avatar Aug 24 '22 16:08 openjdk[bot]

/sponsor

aivanov-jdk avatar Aug 24 '22 16:08 aivanov-jdk

Going to push as commit 568be58e8521e5e87baca1872ba8cc1941607bb7. Since your change was applied there have been 32 commits pushed to the master branch:

  • 69448f9c1a9b123be8ef63bb039111a8976a8da6: 8292679: Simplify thread creation in gtest and port 2 tests to new way
  • 3c2289d65157fca3f303d2fe703b31ef53b2f5bf: 8215916: The failure reason of an optional JAAS LoginModule is not logged
  • 71ab5c95af28497fb31aba8ba9597da71bc4d3d5: 8292816: GPL Classpath exception missing from assemblyprefix.h
  • c0623972cffdbcd7f80e84a1ec344fd382a4a5cc: 8292713: Unsafe.allocateInstance should be intrinsified without UseUnalignedAccesses
  • a45a4b9465d7d01715425000c4fd47c0aa3e76ca: 8292194: G1 nmethod entry barrier disarm value wraps around too early
  • d3fed128671312ae138a7ad5677c3c140de9a8f3: 8292864: Remove resourceHash.hpp from header files
  • b653b9cc274abb906d3ebe887827ef16b4e57be4: 8291969: Convert LoaderConstraintsTable to ResourceHashtable
  • 0813a4705184e33f6bd175dcd9a17d3212890daf: 8282729: Serial: Move BOT implementation to collector specific directory
  • fa5cc4cc8e1c7c1f627905191e311cfcb3d94b5e: 8291878: NMT: Malloc limits
  • ad2e0c4df045261c04b00bfa1faf5c21392edc58: 8292778: EncodingSupport_md.c convertUtf8ToPlatformString wrong placing of free
  • ... and 22 more: https://git.openjdk.org/jdk/compare/aa9b8f04bf74d5fa00f2b27895e7369abea3a930...master

Your commit was automatically rebased without conflicts.

openjdk[bot] avatar Aug 24 '22 16:08 openjdk[bot]

@aivanov-jdk @honkar-jdk Pushed as commit 568be58e8521e5e87baca1872ba8cc1941607bb7.

:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk[bot] avatar Aug 24 '22 16:08 openjdk[bot]