Skip to content
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

8284535 : Fix PrintLatinCJKTest.java test that is failing with Parse Exception #7966

Closed
wants to merge 9 commits into from

Conversation

lawrence-andrew
Copy link
Contributor

@lawrence-andrew lawrence-andrew commented Mar 25, 2022

We need a common manual test framework code that can be shared across all the client manual test. This framework class should have the following

  1. Frame which contains test instruction .
  2. Pass & Fail button so that user can mark the test pass or fail
  3. Upon failing the test user should be able to elaborate why the test failed and this can be added to the test failure.
  4. Disposing of all the frames.

@aivanov-jdk


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issues

  • JDK-8284535: Fix PrintLatinCJKTest.java test that is failing with Parse Exception
  • JDK-8283712: Create a manual test framework class

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7966/head:pull/7966
$ git checkout pull/7966

Update a local copy of the PR:
$ git checkout pull/7966
$ git pull https://git.openjdk.java.net/jdk pull/7966/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7966

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7966.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 25, 2022

👋 Welcome back lawrence-andrew! 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 openjdk bot added the rfr Pull request is ready for review label Mar 25, 2022
@openjdk
Copy link

openjdk bot commented Mar 25, 2022

@lawrence-andrew 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 openjdk bot added the client client-libs-dev@openjdk.org label Mar 25, 2022
@mlbridge
Copy link

mlbridge bot commented Mar 25, 2022

Copy link
Contributor

@prrace prrace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really needs to be delivered in the same PR as something using it so it can actually be tested.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java Outdated Show resolved Hide resolved
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java Outdated Show resolved Hide resolved
@openjdk openjdk bot removed the rfr Pull request is ready for review label Mar 26, 2022
@lawrence-andrew
Copy link
Contributor Author

Fixed PrintLatinCJKTest.java test and used PassFailJFrame for showing test instruction.

@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 26, 2022
@openjdk
Copy link

openjdk bot commented Apr 5, 2022

@lawrence-andrew 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:

8284535: Fix PrintLatinCJKTest.java test that is failing with Parse Exception
8283712: Create a manual test framework class

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 199 new commits pushed to the master branch:

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 openjdk bot added the ready Pull request is ready to be integrated label Apr 5, 2022
@lawrence-andrew
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 5, 2022
@openjdk
Copy link

openjdk bot commented Apr 5, 2022

@lawrence-andrew
Your change (at version 8e8feff) is now ready to be sponsored by a Committer.

Copy link
Member

@aivanov-jdk aivanov-jdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ability to have instructions as HTML text (hosted in JEditorPane) would make the solution more flexible. Obviously, we shouldn't implement everything we can think of. Yet we should still think about possible advanced features.

test/jdk/java/awt/print/PrinterJob/PrintLatinCJKTest.java Outdated Show resolved Hide resolved
test/jdk/java/awt/print/PrinterJob/PrintLatinCJKTest.java Outdated Show resolved Hide resolved
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java Outdated Show resolved Hide resolved
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java Outdated Show resolved Hide resolved
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java Outdated Show resolved Hide resolved
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java Outdated Show resolved Hide resolved
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java Outdated Show resolved Hide resolved
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java Outdated Show resolved Hide resolved
@aivanov-jdk
Copy link
Member

The ability to have instructions as HTML text (hosted in JEditorPane) would make the solution more flexible. Obviously, we shouldn't implement everything we can think of. Yet we should still think about possible advanced features.

Although I had the above in mind, I wanted to suggest adding a timer until the timeout. It could give the tester an idea how much time left.

Shall the timeout be increased to at least 5 minutes? Or more?

A document is to be printed, the printer could be located not close to the tester. There should be enough time to fetch a printed sheet of paper.

@aivanov-jdk
Copy link
Member

This PR includes a test fix. Should the test fix bugid be the main bugid under which the PR is integrated? The bugid for the framework class could be added as an additional resolved bug.

@aivanov-jdk
Copy link
Member

/reviewers 2

@openjdk
Copy link

openjdk bot commented Apr 6, 2022

@aivanov-jdk
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with 1 of role reviewers, 1 of role authors).

@openjdk openjdk bot removed the sponsor Pull request is ready to be sponsored label Apr 6, 2022
@lawrence-andrew
Copy link
Contributor Author

Add a fix where the Test Instruction frame and the test frame can be placed either side by side or up and down.

@mrserb
Copy link
Member

mrserb commented Apr 9, 2022

Just an idea can we replace the builtin in the jtreg "manual" tests machinery by something like this?
Also would like to raise a question: should we show the test instructions in the same VM where the test executed or we can shoiw it in a separate VM?

@aivanov-jdk
Copy link
Member

Just an idea can we replace the builtin in the jtreg "manual" tests machinery by something like this?

An interesting idea. However, this would require updating all the existing tests including those that use its custom UI for run/manual.

Is it worth?
I doubt it…

Also would like to raise a question: should we show the test instructions in the same VM where the test executed or we can show it in a separate VM?

A good idea. The instructions used to be shown in the same VM. And the majority of UI tests don't cause the VM to crash therefore the complexity of separating instructions from the test UI doesn't seem to provide much benefit.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java Outdated Show resolved Hide resolved
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java Outdated Show resolved Hide resolved
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java Outdated Show resolved Hide resolved
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java Outdated Show resolved Hide resolved
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java Outdated Show resolved Hide resolved
final long startTime = System.currentTimeMillis();
timer.setDelay(1000);
timer.addActionListener((e) -> {
int leftTime = testTimeout - (int) (System.currentTimeMillis() - startTime);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

long timeLeft is better.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java Outdated Show resolved Hide resolved
* @bug 800535 8022536
* @bug 8022536
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is 800535 removed? It's the bug this test is written for.

The additional bug 8022536 may cause NPE on Linux if a (default) remote printer is unavailable.

test/jdk/java/awt/print/PrinterJob/PrintLatinCJKTest.java Outdated Show resolved Hide resolved
Comment on lines 51 to 52
private static volatile boolean failed = false;
private static volatile boolean timeout = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

false is the default value, so you can skip assigning false.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java Outdated Show resolved Hide resolved
@openjdk openjdk bot removed the rfr Pull request is ready for review label Apr 11, 2022
@openjdk openjdk bot added the rfr Pull request is ready for review label Apr 11, 2022
@openjdk openjdk bot added the ready Pull request is ready to be integrated label Apr 11, 2022
@lawrence-andrew
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 11, 2022
@openjdk
Copy link

openjdk bot commented Apr 11, 2022

@lawrence-andrew
Your change (at version 07735ce) is now ready to be sponsored by a Committer.

@mrserb
Copy link
Member

mrserb commented Apr 12, 2022

An interesting idea. However, this would require updating all the existing tests including those that use its custom UI for run/manual.

Isn't this a goal of this fix:

We need a common manual test framework code that can be shared across all the client manual test.
?

A good idea. The instructions used to be shown in the same VM. And the majority of UI tests don't cause the VM to crash therefore the complexity of separating instructions from the test UI doesn't seem to provide much benefit

It means that the java options such as L&F selection/etc will affect the instruction.

@prrace
Copy link
Contributor

prrace commented Apr 12, 2022

Just an idea can we replace the builtin in the jtreg "manual" tests machinery by something like this?

It might be a nice project to update all of the rest of the tests but it'll be incremental and should start with tests we need to update anyway.

Also would like to raise a question: should we show the test instructions in the same VM where the test executed or we can shoiw it in a separate VM?

Separate VM would seem to introduce a lot of questions about how results are communicated and how you launch the target which file is really the @test ... if someone were to introduce such a helper class along side the one here and let test developers choose which one they want for a while, we could perhaps see which approach works out better. But that would be an exercise outside of the scope of this proposal.

@aivanov-jdk
Copy link
Member

An interesting idea. However, this would require updating all the existing tests including those that use its custom UI for run/manual.

Isn't this a goal of this fix:

We need a common manual test framework code that can be shared across all the client manual test.
?

Doesn't this accomplish this goal?

No, it doesn't replace the (outdated) built-in machinery in jtreg for manual=yesno. At the same time, it provides a building block to update those tests.

I like this approach better, in which a test can be easily run without the need for jtreg, I think it greatly simplifies debugging and updating the test.

However, you're right that applet/manual=yesno in jtreg can be reimplemented using this new class PassFailJFrame even though it's not a goal at this very time. Initially I interpreted your suggestion as replacing all manual tests, which doesn't seem a right thing to do because it will break all existing manual tests.

@aivanov-jdk
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented Apr 12, 2022

Going to push as commit 3f26d84.
Since your change was applied there have been 210 commits pushed to the master branch:

  • a5378fb: 8284125: AArch64: Remove partial masked operations for SVE
  • 8346643: 8196465: javax/swing/JComboBox/8182031/ComboPopupTest.java fails on Linux
  • 5851631: 8284622: Update versions of some Github Actions used in JDK workflow
  • 4cd0921: 8284653: Serial: Inline GenCollectedHeap::collect_locked
  • 9545ba7: 8282716: [macos] Enable javax/swing/JScrollPane/TestMouseWheelScroll.java on macos
  • 4ce3cf1: 8283245: Create a test for JDK-4670319
  • fad3b94: 8282640: Create a test for JDK-4740761
  • 4e165f6: 8284308: mismatch between key and content in compiler error message
  • 4d45c3e: 8284620: CodeBuffer may leak _overflow_arena
  • 73aa555: 8284689: ProblemList java/lang/Integer/Unsigned.java in -Xcomp mode
  • ... and 200 more: https://git.openjdk.java.net/jdk/compare/f4fd53d0aee67319bf2c7bcaa671c2e97e66383f...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Apr 12, 2022
@openjdk openjdk bot closed this Apr 12, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 12, 2022
@openjdk
Copy link

openjdk bot commented Apr 12, 2022

@aivanov-jdk @lawrence-andrew Pushed as commit 3f26d84.

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

@lawrence-andrew lawrence-andrew deleted the JDK-8283712 branch April 12, 2022 14:25
@mrserb
Copy link
Member

mrserb commented Apr 13, 2022

I like this approach better, in which a test can be easily run without the need for jtreg, I think it greatly simplifies debugging and updating the test.

The new code depends on the PassFailJFrame from the library so it is not a simple test-in-one-file anyway. The jtreg could be just a wrapper that shows the instructions from the txt file and initialized/run the test via the main method, so the test will not have any external dependenies. I just worry that we added one more library class and eventually will have one more way to write a manual test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client client-libs-dev@openjdk.org integrated Pull request has been integrated
4 participants