8319555: [TestBug] Utility for creating instruction window for manual tests #1413

andy-goryachev-oracle · 2024-03-20T20:21:30Z

ManualTestWindow

This facility provides the base class for manual tests which displays the test instructions,
the UI under test, and the Pass/Fail buttons.

Example:

public class ManualTestExample extends ManualTestWindow {
    public ManualTestExample() {
        super(
            "Manual Test Example",
            """
            Instructions:
            1. you will see a button named "Test"
            2. press the button
            3. verify that the button can be pressed""",
            400, 250
        );
    }

    public static void main(String[] args) throws Exception {
        launch(args);
    }

    @Override
    protected Node createContent() {
        return new Button("Test");
    }
}

Resulting application window:

Readme:

https://github.com/andy-goryachev-oracle/jfx/blob/8319555.manual/tests/manual/util/README.md

@prrace 's test EmojiTest has been converted to use the new test window as a demonstration (also fixed the Eclipse project so it works now).

Q: What other features can be added to the test window?

Edit: the sources are left in their original place at the root of the project.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8319555: [TestBug] Utility for creating instruction window for manual tests (Enhancement - P4)

Reviewers

Alexander Matveev (@sashamatveev - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1413

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1413.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-03-20T20:21:48Z

👋 Welcome back angorya! 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.

openjdk · 2024-03-20T20:21:51Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

mlbridge · 2024-03-21T16:12:37Z

Webrevs

kevinrushforth · 2024-03-26T18:26:00Z

Not a review, but just a general comment or two.

EmojiTest has been converted to use the new test window as a demonstration

That seems reasonable, but...

(also fixed the Eclipse project and moved sources to src/, still using the default package).

I'm not sure how much value there is in moving the files under a "src" directory without also providing a build script (ant or gradle) to be able to build it. We don't currently have the manual tests wired up to the build, so there is no good way to depend on anything outside of the directory in question (at least not without using an IDE, which is a developer convenience, not a build system). That should probably be solved first.

Q: Do we want to avoid using the default package?

For all manual tests? Probably not.

andy-goryachev-oracle · 2024-03-26T18:30:04Z

I'm not sure how much value there is in moving the files under a "src" directory

Thank you Kevin for the feedback! I had problems configuring the project with sources being in the root, but will take another look.

openjdk · 2024-04-08T15:01:18Z

@andy-goryachev-oracle Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

andy-goryachev-oracle · 2024-04-08T15:04:21Z

Moved the sources back to the root.

Eclipse has a weird behavior when it comes to modular projects or mixing modular and classpath sources: the project which has been modified (or converted to modular or back) needs to be closed and re-opened, otherwise Eclipse loses its marbles. Both 2023-06 and 2024-03 versions seem to exhibit this behavior.

andy-goryachev-oracle · 2024-04-08T17:38:56Z

@mstr2 @jperedadnr @sashamatveev @aghaisas
would you gentlemen be interested in reviewing this?

kevinrushforth · 2024-04-08T22:11:57Z

This could use a second pair of eyes. I don't have time right now, but I'll add a couple comments.

/reviewers 2

openjdk · 2024-04-08T22:12:54Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

kevinrushforth

I left one general comment inline. I also have a question about how to build this without a build script given that the utility is in a separate directory tree from the test application (EmojiText) that you updated to use this?

At a minimum, you need instructions for compiling and running it on the command line, but it might be better to defer this until we can wire up the manual tests to the build (which would be a good test sprint task).

kevinrushforth · 2024-04-08T22:15:04Z

tests/manual/text/EmojiTest.java

-
-        """;
-
+public class EmojiTest {


Our existing manual tests all extend Application. Is there a reason this has to change? Absent a compelling reason, I'd prefer to not require test apps to stop extending Application in order to use this utility.

Everything, including creating an instance of Application, is taken care of by the ManualTestWindow. This simplifies the test code greatly.

What is the reason that the test must extend Application?

For example if test needs to access getParameters() or getHostServices () from Application how it can be done? I think it might be better to extend ManualTestWindow from Application and require test itself to extend ManualTestWindow. In this case tests will extend Application.

sashamatveev · 2024-04-08T22:23:07Z

tests/manual/util/README.md

+                 Provide
+                 multi-line instructions here.
+                 """
+             ).


Add size(1200, 800). to example. I think adjusting size will be common enough and no need to look at ManualTestWindow to figure it out.

good point!

sashamatveev · 2024-04-08T22:24:55Z

tests/manual/util/README.md

+}
+```
+
+Resulting application window:


Resulting application window does not look like it is from example above. Not sure if it is important.

Not really important.

Apart from being resized to decrease the size of the PNG image, it looks exactly the same. Are you running it on a different platform? How does it look, anything appears missing (can you post a screenshot)?

If you run EmojiTest, then yes it will look correct, but if you run SampleManualTest it will not. Since you added this image to SampleManualTest as resulting image of SampleManualTest. Look at ReadMe.md.

indeed! thanks!

sashamatveev · 2024-04-08T22:30:07Z

tests/manual/util/src/com/oracle/util/testing/ManualTestWindow.java

+// TODO timeout
+// TODO log area
+// TODO screenshots on failure?
+// TODO initial position?


In general I do not like TODO in comments. You can file follow up issue to improve/extend ManualTestWindow if needed.

sashamatveev · 2024-04-08T22:40:17Z

tests/manual/util/src/com/oracle/util/testing/ManualTestWindow.java

+            stage.setWidth(width == 0 ? 800 : width);
+            stage.setHeight(height == 0 ? 600 : height);


I think width and height is double and not int. Also, lets not change it. If test wants 0, then pass 0.

andy-goryachev-oracle · 2024-04-10T23:02:18Z

Thank you all for comments and suggestions!
Changed to extend Application. See if you like this better.

sashamatveev

Only minor comment about unused code. Otherwise looks fine to me.

sashamatveev · 2024-04-10T23:27:32Z

tests/manual/util/src/com/oracle/util/testing/ManualTestWindow.java

+        /*
+        Button screenshotButton = new Button("Screenshot");
+        screenshotButton.setMinWidth(100);
+        screenshotButton.setOnAction((ev) -> {
+        });
+        screenshotButton.setDisable(true);
+        */


I will suggest to delete unused code.

sashamatveev · 2024-04-10T23:27:48Z

tests/manual/util/src/com/oracle/util/testing/ManualTestWindow.java

+
+        HBox buttons = new HBox(
+            10,
+            //screenshotButton,


openjdk · 2024-04-10T23:42:10Z

⚠️ @andy-goryachev-oracle This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

andy-goryachev-oracle · 2024-04-10T23:42:20Z

it's in the test code, but ok - removed commented out code. thanks!

bridgekeeper · 2024-05-17T19:15:15Z

@andy-goryachev-oracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

andy-goryachev-oracle · 2024-05-17T19:19:53Z

keep-alive

mstr2 · 2024-05-23T16:01:29Z

I run manual tests on the command line like so:

java --module-path=$env:JFX_SDK --add-modules=javafx.controls ./tests/manual/events/PlatformPreferencesChangedTest.java

This doesn't work with the new utility class, since it is outside of the class file hierarchy.
Short of using a build system, how would I run a manual test with this new class?

andy-goryachev-oracle · 2024-05-23T16:11:42Z

Short of using a build system, how would I run a manual test with this new class?

I am running from within Eclipse. Here is the command line (remove newlines) it launches the EmojiTest. I think the key is to add the location of compiled classes via -classpath:

/Library/Java/JavaVirtualMachines/jdk-21.jdk/Contents/Home/bin/java
-XX:+ShowCodeDetailsInExceptionMessages
-Dfile.encoding=UTF-8
-Dstdout.encoding=UTF-8
-Dstderr.encoding=UTF-8
-p /Users/angorya/Projects/jfx-1/jfx/rt/modules/javafx.base/bin:/Users/angorya/Projects/jfx-1/jfx/rt/modules/javafx.graphics/bin:/Users/angorya/Projects/jfx-1/jfx/rt/modules/javafx.controls/bin
-classpath /Users/angorya/Projects/jfx-1/jfx/rt/tests/manual/util/bin:/Users/angorya/Projects/jfx-1/jfx/rt/tests/manual/text/bin
--add-reads javafx.base=java.management
--add-reads javafx.base=jdk.management
--add-exports javafx.base/com.sun.javafx.property=javafx.graphics
--add-exports javafx.base/test.javafx.collections=javafx.graphics
--add-exports javafx.base/test.util.memory=javafx.graphics
--add-exports java.base/sun.security.util=javafx.graphics
--add-reads javafx.base=java.management
--add-reads javafx.base=jdk.management
--add-exports javafx.graphics/test.com.sun.javafx.pgstub=javafx.controls
--add-exports javafx.base/test.com.sun.javafx.binding=javafx.controls
--add-exports javafx.base/test.util.memory=javafx.controls
--add-exports javafx.base/test.javafx.collections=javafx.controls
--add-exports javafx.base/com.sun.javafx.property=javafx.graphics
--add-exports javafx.base/test.javafx.collections=javafx.graphics
--add-exports javafx.base/test.util.memory=javafx.graphics
--add-exports java.base/sun.security.util=javafx.graphics
--add-reads javafx.base=java.management
--add-reads javafx.base=jdk.management
--add-modules=javafx.base,javafx.graphics,javafx.controls
--add-opens javafx.controls/test.com.sun.javafx.scene.control.test=javafx.base
--add-exports javafx.base/com.sun.javafx=ALL-UNNAMED
-Djava.library.path=../../../build/sdk/lib EmojiTest

(also, there might be some unnecessary -add-exports here as well)

andy-goryachev-oracle · 2024-05-23T16:12:41Z

But this is a good question: is there a better way to organize the tests to make it easier to use common code?

mstr2 · 2024-05-23T16:25:39Z

Short of using a build system, how would I run a manual test with this new class?

I am running from within Eclipse. Here is the command line (remove newlines) it launches the EmojiTest. I think the key is to add the location of compiled classes via -classpath:

Yes this will work, but it requires me to compile ManualTestWindow.java before. That's quite a lot of heavy-lifting without tooling support (like you get from Eclipse). Maybe we should integrate the manual tests into the Gradle build, so that running a manual test might be as easy as invoking the application plugin's :run task for the manual test.

kevinrushforth · 2024-05-23T17:38:25Z

We need to provide a build script for any manual test that isn't self-contained in a single directory (see my earlier comment). For example, we have a build script for MonkeyTester and FXMediaPlayer.

As a separate step, we could make it available as part of the gradle build, as long as we can do it without adding individual tests to build.gradle (sort of like we do for apps), but the previous is the minimum.

This should probably be put on the back burner until the next test sprint.

andy-goryachev-oracle · 2024-05-23T17:46:42Z

or possibly build the common test code into a jar as a part of the standard build and include it along with -Djava.library.path?

andy-goryachev-oracle · 2024-05-23T21:05:51Z

moving to DRAFT until the next test sprint (after jfx23 ships).

andy-goryachev-oracle added 3 commits March 19, 2024 13:16

fixed text manual test setup

565ef1c

manual test window

2524eea

Merge branch 'master' into 8319555.manual

5aba674

whitespace

1cc0950

andy-goryachev-oracle marked this pull request as ready for review March 21, 2024 16:08

openjdk bot added the rfr Ready for review label Mar 21, 2024

andy-goryachev-oracle added 3 commits April 8, 2024 07:31

Merge branch 'master' into 8319555.manual

22da890

sources moved back

66d7d0d

.

7e942a3

andy-goryachev-oracle force-pushed the 8319555.manual branch from 66d7d0d to 22da890 Compare April 8, 2024 14:50

works

a106587

kevinrushforth reviewed Apr 8, 2024

View reviewed changes

sashamatveev reviewed Apr 8, 2024

View reviewed changes

extends application

5e2e993

openjdk bot removed the rfr Ready for review label Apr 10, 2024

whitespace

dbcdb54

openjdk bot added the rfr Ready for review label Apr 10, 2024

removed example

76a4dc8

sashamatveev reviewed Apr 10, 2024

View reviewed changes

andy-goryachev-oracle added 2 commits April 10, 2024 16:40

cleanup

a83a81f

Merge remote-tracking branch 'origin/8319555.manual' into 8319555.manual

a8fc661

sashamatveev approved these changes Apr 10, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into 8319555.manual

45e5310

andy-goryachev-oracle mentioned this pull request Apr 19, 2024

8330701: Fix Eclipse project in tests/manual/text #1443

Closed

3 tasks

Merge branch 'master' into 8319555.manual

e4c8d29

andy-goryachev-oracle marked this pull request as draft May 23, 2024 21:05

openjdk bot removed the rfr Ready for review label May 23, 2024

		stage.setWidth(width == 0 ? 800 : width);
		stage.setHeight(height == 0 ? 600 : height);

8319555: [TestBug] Utility for creating instruction window for manual tests #1413

Are you sure you want to change the base?

8319555: [TestBug] Utility for creating instruction window for manual tests #1413

Conversation

andy-goryachev-oracle commented Mar 20, 2024 • edited by openjdk bot

ManualTestWindow

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Mar 20, 2024

openjdk bot commented Mar 20, 2024

mlbridge bot commented Mar 21, 2024 • edited

Webrevs

kevinrushforth commented Mar 26, 2024

andy-goryachev-oracle commented Mar 26, 2024

openjdk bot commented Apr 8, 2024

andy-goryachev-oracle commented Apr 8, 2024

andy-goryachev-oracle commented Apr 8, 2024

kevinrushforth commented Apr 8, 2024

openjdk bot commented Apr 8, 2024

kevinrushforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-goryachev-oracle commented Apr 10, 2024

sashamatveev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Apr 10, 2024

andy-goryachev-oracle commented Apr 10, 2024

bridgekeeper bot commented May 17, 2024

andy-goryachev-oracle commented May 17, 2024

mstr2 commented May 23, 2024

andy-goryachev-oracle commented May 23, 2024 • edited

andy-goryachev-oracle commented May 23, 2024

mstr2 commented May 23, 2024

kevinrushforth commented May 23, 2024

andy-goryachev-oracle commented May 23, 2024

andy-goryachev-oracle commented May 23, 2024

andy-goryachev-oracle commented Mar 20, 2024 •

edited by openjdk bot

mlbridge bot commented Mar 21, 2024 •

edited

andy-goryachev-oracle commented May 23, 2024 •

edited