-
Notifications
You must be signed in to change notification settings - Fork 5.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8294156: Allow PassFailJFrame.Builder to create test UI #15665
8294156: Allow PassFailJFrame.Builder to create test UI #15665
Conversation
Instructions for FileChooserSymLinkTest are rendered in HTML.
Each text component (JTextArea or JEditorPane) are configured in its own dedicated method. This avoids unnecessary type casts.
It reverts the accidental change where I unwrapped call to createUI
Moved it to a named class, saved the instance in a field. It's to be used with test windows.
The Builder now provides a method to create test UI, PassFailJFrame automatically invokes the method reference on the EDT and then shows both the instruction frame and the test window.
Also clean up the doc for disposeWindows().
The WindowClosingHandler is used both for the instruction frame and for test UI windows. Also add blank lines to separate different parts of the class.
It's not supposed to be extended.
👋 Welcome back aivanov! A progress list of the required criteria for merging this PR into |
@aivanov-jdk The following label will be automatically applied to this pull request:
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. |
Webrevs
|
|
||
@FunctionalInterface | ||
public interface WindowCreator { | ||
Window createTestUI(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes we may need to create more than one window(e.g. some DnD test with two windows).
Perhaps we should consider that here by adding List<Window> createTestUI()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes we may need to create more than one window(e.g. some DnD test with two windows). Perhaps we should consider that here by adding
List<Window> createTestUI()
.
I thought about it. Yet I couldn't find a simple scenario where multiple windows need to be created. When there are multiple windows, you have to position them. The positions of the windows may depend on the position of the instruction frame. In #12447, Harshitha @honkar-jdk managed many test windows.
I decided to handle the most common scenario: one test window.
This can be extended in the future by creating WindowListCreator
interface, its createTestUI
method will return a list or a collection: List<Window>
or Collection<Window>
. Changing the signature of the existing WindowCreator.createTestUI
could also be possible without breaking backwards compatibility.
Now that I think about it more, you have a point… Why not allow passing a list of windows right away? However, to test this case I'll need a (simple) scenario where multiple windows are created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, to test this case I'll need a (simple) scenario where multiple windows are created.
For example test/jdk/java/awt/event/MouseEvent/SpuriousExitEnter/SpuriousExitEnter_2.java
is not converted to use PassFailJFrame, but shows instruction window and two test windows.
For these test windows, we can safely reduce the width a lot and arrange them in a row.
Probably we could add some other sophisticated layouts later, e.g.:
But this kind of layout seems to be beyond the scope of this PR, I just wish we hadn't missed the option to add multiple windows at once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But laying out the test windows is really a problem. With one window, the framework already provides a way to position the window, it is applied to the primary test window.
Before showing other windows, they need to be positioned.
There could be a callback so that the test developer is able to position the windows. In the future, we may add simple layouts to perform this task automatically.
But this kind of layout seems to be beyond the scope of this PR, I just wish we hadn't missed the option to add multiple windows at once.
I agree, it's better to implement it right away. I didn't think about it as viable solution because of positioning issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Dispose all the window(s) i,e both the test instruction frame and | ||
* the window(s) that is added via addTestWindow(Window testWindow) | ||
* Disposes of all the windows. It disposes of the test instruction frame | ||
* and all other windows added via {@link #addTestWindow(Window)}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we mention here a window added by testUI
via builder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we mention here a window added by
testUI
via builder?
I don't think it's necessary: the builder uses addTestWindow
under the hood. It's the builder that requires documentation.
In fact, the entire class requires a bit of TLC for documentation. I even wrote some after Lawrence @lawrence-andrew created the first version of PassFailJFrame
but I never finished it. Even this part was implemented nearly a year ago.
I plan to create a wiki page on the client-libs wiki to describe creating manual tests with PassFailJFrame
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we mention here a window added by
testUI
via builder?I don't think it's necessary: the builder uses
addTestWindow
under the hood. It's the builder that requires documentation.In fact, the entire class requires a bit of TLC for documentation. I even wrote some after Lawrence @lawrence-andrew created the first version of
PassFailJFrame
but I never finished it. Even this part was implemented nearly a year ago.I plan to create a wiki page on the client-libs wiki to describe creating manual tests with
PassFailJFrame
.
Just saw this, and it mirrors what I wrote in another PR that we need docs, but I believe they are best placed inside the source itself. Not everyone who can update this source can update the wiki.
That doesn't preclude you from putting more examples on the wiki and a reference to it from the source but I still think there should be docs in the source.
Include how to use this feature and something like to the builder example you have here might be good too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, I started writing javadoc for PassFailJFrame
while Lawrence and I discussed it. I explained how to use the framework. But I have never finished it to a PR-ready state.
As I noted in the HTML enhancement PR, I'd rather document both features together.
On the other hand, I would prefer integrating this so that people could use it already. In my opinion, this feature streamlines the usage of PassFailJFrame
.
At the same time, the framework needs docs badly, especially as such rich features are added. And writing docs usually slips as other tasks come up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prrace I've just pushed documentation on how to use PassFailJFrame
. It provided two samples: the new one with the builder pattern and the classic one. The samples can be copied and pasted into a file, they're ready to run, except for imports and jtreg tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also submitted a bug to provide more comprehensive documentation:
JDK-8318688: Write documentation for PassFailJFrame
The windows can be positioned in advance, or a call back PositionWindows interface can be used to define their positions after the instruction UI frame is positioned on the screen.
The new several test UI windows , looks good to me. |
I submitted an enhancement JDK-8317116: Provide layouts for multiple test UI in |
The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork: git checkout 8294156-func-manual-TestUI
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push |
@aivanov-jdk this pull request can not be integrated into git checkout 8294156-func-manual-TestUI
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
@aivanov-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:
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 no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
/integrate |
Going to push as commit 42b9ac8. |
@aivanov-jdk Pushed as commit 42b9ac8. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This enhances the
Builder
pattern added in JDK-8294535 with a new methodtestUI
which allows passing a lambda expression or a method reference to create the test UI window.The
PassFailJFrame
will automatically call the method on the EDT to create the UI, add it to the internal list of windows, install the window closing listener and finally position and show both the instructions and test UI.Alternatively, you can pass an already created window.
The
main
method of a manual test could look as simple as a sequence of calls:where
createTestUI
returns a test UI window.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15665/head:pull/15665
$ git checkout pull/15665
Update a local copy of the PR:
$ git checkout pull/15665
$ git pull https://git.openjdk.org/jdk.git pull/15665/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 15665
View PR using the GUI difftool:
$ git pr show -t 15665
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15665.diff
Webrev
Link to Webrev Comment