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

8273634: [TEST_BUG] Improve javax/swing/text/ParagraphView/6364882/bug6364882.java #5661

Closed
wants to merge 1 commit into from

Conversation

aivanov-jdk
Copy link
Member

@aivanov-jdk aivanov-jdk commented Sep 23, 2021

The test is being moved from the closed to open.

I updated the bug6364882.java test so that it's headless now. If it fails, the image of the editorPane is saved for analysis. I also added more asserts which verify:

• the first three lines are aligned to the right edge;
• the fourth line isn't justified and its width is less than 3/4 of the width;
• the fifth and sixth lines have width less than that of the fourth line and have the same width.

There's an option to show the UI for visual inspection as well as to save the image even if the test passes; use -show and/or -save parameters to the test correspondingly.

[1] JDK-6364882: Last line of a paragraph should not be justified


Progress

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

Issue

  • JDK-8273634: [TEST_BUG] Improve javax/swing/text/ParagraphView/6364882/bug6364882.java

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5661

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 23, 2021

👋 Welcome back aivanov! 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 Sep 23, 2021
@openjdk
Copy link

openjdk bot commented Sep 23, 2021

@aivanov-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 openjdk bot added the client client-libs-dev@openjdk.org label Sep 23, 2021
@mlbridge
Copy link

mlbridge bot commented Sep 23, 2021

Webrevs

Copy link
Member

@mrserb mrserb left a comment

Choose a reason for hiding this comment

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

I assume that an updated test still fail before JDK-6364882

@openjdk
Copy link

openjdk bot commented Sep 24, 2021

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

8273634: [TEST_BUG] Improve javax/swing/text/ParagraphView/6364882/bug6364882.java

Reviewed-by: serb

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 1 new commit pushed to the master branch:

  • 56b8b35: 8273261: Replace 'while' cycles with iterator with enhanced-for in java.base

Please see this link for an up-to-date comparison between the source branch of this pull request and 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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Sep 24, 2021
@aivanov-jdk
Copy link
Member Author

I assume that an updated test still fail before JDK-6364882

Correct. I forgot to add this statement. The updated test fails with builds of JDK 6 until JDK-6364882 is fixed, that is the test fails when run with 6-b75 and passes when run with 6-b76.

In this way the test and PR are related to JDK-8239502 and #5661 correspondingly.

@aivanov-jdk
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Sep 28, 2021

Going to push as commit 67e52a3.
Since your change was applied there have been 53 commits pushed to the master branch:

  • b7425b6: 8239502: [TEST_BUG] Test javax/swing/text/FlowView/6318524/bug6318524.java never fails
  • c57a6c6: 8274265: Suspicious string concatenation in logTestUtils.inline.hpp
  • 6f4cefb: 8274394: Use Optional.isEmpty instead of !Optional.isPresent in jdk.jlink
  • 94f5e80: 8274276: Cache normalizedBase URL in URLClassPath.FileLoader
  • b36881f: 8274383: JNI call of getAccessibleSelection on a wrong thread
  • be40373: 8274381: missing CAccessibility definitions in JNI code
  • 79865cd: 8274259: G1: assert(check_alignment(result)) failed: address not aligned: 0x00000008baadbabe after JDK-8270009
  • 961dcff: 8273581: Change the mechanism by which JDK loads the platform-specific FontManager class
  • 6a573b8: 8273508: Support archived heap objects in SerialGC
  • 3eca9c3: 8264707: HotSpot Style Guide should permit use of lambda
  • ... and 43 more: https://git.openjdk.java.net/jdk/compare/0aa63feca8704c8958530ef9e3df128570c50e12...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 28, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 28, 2021
@openjdk
Copy link

openjdk bot commented Sep 28, 2021

@aivanov-jdk Pushed as commit 67e52a3.

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

@prrace
Copy link
Contributor

prrace commented Sep 29, 2021

This test looks headless.
It uses frame only if an arg is passed clearly for manual debugging and the test config doesn't use it.
"Our GUI platforms" being what ?
And the information being pasted in here should be in the bug report.
Even then there's nothing to help someone figure out why it passed for others and not you.
What font was in use ? Why do you think headful made a difference ?

@prrace
Copy link
Contributor

prrace commented Sep 29, 2021

I've looked at the test a tad more and two comments

  1. if it fails, it dumps an image. You can provide that image
  2. It is sensitive to the actual font. If your default font for whatever reason is of a size thatonemore or one less word will fit on a line, then it will fail. This does look like a test problem to me.

@aivanov-jdk
Copy link
Member Author

Thank you, @prrace, for your comments.

  1. Unfortunately, it doesn't save the image if an exception is thrown. I overlooked this fact.
  2. I agree I should've made it more flexible to accommodate for possible font size differences.

I'll fix these issues and create my own PR for JDK-8274465.

@prrace
Copy link
Contributor

prrace commented Sep 29, 2021

"Unfortunately, it doesn't save the image if an exception is thrown. I overlooked this fact."

oops :-)

@aivanov-jdk aivanov-jdk deleted the JDK-8273634 branch February 21, 2022 14:17
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
3 participants