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

8286620: Create regression test for verifying setMargin() of JRadioButton #8721

Closed
wants to merge 19 commits into from

Conversation

TejeshR13
Copy link
Contributor

@TejeshR13 TejeshR13 commented May 16, 2022

Added test for checking setMargin() of JRadioButton.


Progress

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

Issue

  • JDK-8286620: Create regression test for verifying setMargin() of JRadioButton

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8721

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented May 16, 2022

👋 Welcome back TejeshR13! 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
Copy link

openjdk bot commented May 16, 2022

⚠️ @TejeshR13 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).

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

openjdk bot commented May 16, 2022

@TejeshR13 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 May 16, 2022
@mlbridge
Copy link

mlbridge bot commented May 16, 2022

@TejeshR13 TejeshR13 changed the title 8286620: Move bug4380543 from closed to open 8286620: Added test for checking setMargin() of JRadioButton May 16, 2022
@TejeshR13 TejeshR13 changed the title 8286620: Added test for checking setMargin() of JRadioButton 8286620: Create regression test for verifying setMargin() of JRadioButton May 19, 2022
@openjdk
Copy link

openjdk bot commented May 19, 2022

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

8286620: Create regression test for verifying setMargin() of JRadioButton

Reviewed-by: jdv, honkar, 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 178 new commits pushed to the master branch:

  • 062db59: 8286206: Missing cases for RECORD
  • ee4a6c2: 8287799: JFR: Less noisy platform threads with jfr print
  • 1499e5e: 8273573: [macos12] ActionListenerCalledTwiceTest.java fails on macOS 12
  • 2f62f15: 8287808: javac generates illegal class file for pattern matching switch with records
  • 905bcbe: 8286571: java source launcher from a minimal jdk image containing jdk.compiler fails with --enable-preview option
  • 8d28734: 8287741: Fix of JDK-8287107 (unused cgv1 freezer controller) was incomplete
  • f1dd559: 8287896: PropertiesTest.sh fail on msys2
  • 4fe0ca9: 8287860: Revise usage of volatile in j.u.Locale
  • bde7a7a: 8287236: Reorganize AST related to pattern matching for switch
  • 2d8c649: 8287663: Add a regression test for JDK-8287073
  • ... and 168 more: https://git.openjdk.java.net/jdk/compare/e9bddc18ab91c29d491b0e3bd145d641f6a62c5d...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 (@jayathirthrao, @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 May 19, 2022
@mrserb
Copy link
Member

mrserb commented May 24, 2022

Just an idea for discussion: I remember that we added some support for the manual tests, kind of framework or something. Probably it will be useful to reuse when we add "new" tests? https://bugs.openjdk.java.net/browse/JDK-8283803

@TejeshR13
Copy link
Contributor Author

Just an idea for discussion: I remember that we added some support for the manual tests, kind of framework or something. Probably it will be useful to reuse when we add "new" tests? https://bugs.openjdk.java.net/browse/JDK-8283803

Means you are suggesting to use the passfailFrame?

@aivanov-jdk
Copy link
Member

Just an idea for discussion: I remember that we added some support for the manual tests, kind of framework or something. Probably it will be useful to reuse when we add "new" tests? https://bugs.openjdk.java.net/browse/JDK-8283803

Means you are suggesting to use the passfailFrame?

Why not? The framework allows you to re-use the functionality for creating the UI and for handling Pass/Fail buttons instead of creating your own.

@honkar-jdk
Copy link
Contributor

@TejeshR13 As mentioned by @aivanov-jdk, @mrserb PassFailJFrame Framework, allows you to create the required UI for manual tests and automatically handles the disposal of any test resources.

@TejeshR13
Copy link
Contributor Author

Just an idea for discussion: I remember that we added some support for the manual tests, kind of framework or something. Probably it will be useful to reuse when we add "new" tests? https://bugs.openjdk.java.net/browse/JDK-8283803

Thank you for the suggestion @mrserb , it did good.

@honkar-jdk
Copy link
Contributor

Tested on Win10, visually the insets look fine. Minor formatting changes suggested.

@aivanov-jdk
Copy link
Member

@aivanov-jdk Recently we have started to use the latter and remove the extra folder (/4380543/)

Thank you for clarification, Harshitha.

Yes, blame (?) me :-)
We may not always remember to point it out but it is what we want.

I see. I noticed it in recent code reviews, so I wanted to clarify.

I've never seen the point in an extra level of folder except when the test is composed of multiple files all unique to the test.

I agree to some extent… When using an IDE to compile and run a test, a separate folder is quite convenient: add it as test source, and you're done. When there are many tests in one folder, it could result in compilation errors. Some tests declare the same class names, a number of manual tests have Sysout class, and thus it causes duplicate class names, which makes it impossible to run a test right from IDE unless you take additional steps to resolve or prevent compilation errors.

On the other hand, the shorter filesystem tree is easier to navigate. As such, flatter structure is the way to go.

I also request that new tests not be given names like bug87654321.java but instead be named in a way that you can tell what they are supposed to be testing like in this case something like RadioButtonMarginTest.java

Descriptive names are easier to remember and to refer to, they give you an idea of what test does when you see a failure. With a bugid, you have to open the test source or the bug in JBS for getting that info.

I fully support meaningful, descriptive names for new tests.

@TejeshR13
Copy link
Contributor Author

I can see no reason why the test is for Windows only.
The original bug JDK-4380543 lists all the operating systems as affected. Then JDK-8134640 asked to support all Look-and-Feels, which somewhat implies other platforms should also be tested.

Ok. Since this PR is about the movement of test from closed to open, I would like to retain it for Windows alone @aivanov-jdk as the bug fix JDK-8134640 has been taken care for windows. Will surely plan for other L&F in sometime.

Why can't we make it run on other platforms here?

All it takes is removing @requires (os.family == "windows"). Well, obviously, you have to run it on macOS and Linux to make sure it works as expected.

I think doing it right now is more than reasonable compared to submitting a new issue just to remove the OS restriction and going through the code review.

Sorry, had really misunderstood what you had meant, removing os specific is a great idea. Thank you @aivanov-jdk .

@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 2, 2022
@openjdk openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 2, 2022
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.

Except for the minor comments, it looks good now.

test/jdk/javax/swing/JRadioButton/bug4380543.java Outdated Show resolved Hide resolved
test/jdk/javax/swing/JRadioButton/bug4380543.java Outdated Show resolved Hide resolved
@mlbridge
Copy link

mlbridge bot commented Jun 5, 2022

Mailing list message from Justin Senseney on client-libs-dev:

Please unsubscribe me from your mailing list

On Sat, Jun 4, 2022, 20:01 Tejesh R <tr at openjdk.java.net> wrote:

@mlbridge
Copy link

mlbridge bot commented Jun 7, 2022

Mailing list message from Aleksei Ivanov on client-libs-dev:

Hi Justin,

To unsubscribe from any OpenJDK mailing lists, go to its page; for
client-libs-dev, go to:

https://mail.openjdk.java.net/mailman/listinfo/client-libs-dev

Scroll to the bottom of the page, enter your email address in the
section "To unsubscribe from client-libs-dev?" and click "Unsubscribe or
edit options". Follow the instructions in the message you receive.

On 05/06/2022 01:10, Justin Senseney wrote:

Please unsubscribe me from your mailing list

--
Regards,
Alexey

test/jdk/javax/swing/JRadioButton/bug4380543.java Outdated Show resolved Hide resolved
test/jdk/javax/swing/JRadioButton/bug4380543.java Outdated Show resolved Hide resolved
@TejeshR13
Copy link
Contributor Author

/integrate

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

openjdk bot commented Jun 7, 2022

@TejeshR13
Your change (at version 631e6e2) is now ready to be sponsored by a Committer.

@aivanov-jdk
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented Jun 7, 2022

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

  • 062db59: 8286206: Missing cases for RECORD
  • ee4a6c2: 8287799: JFR: Less noisy platform threads with jfr print
  • 1499e5e: 8273573: [macos12] ActionListenerCalledTwiceTest.java fails on macOS 12
  • 2f62f15: 8287808: javac generates illegal class file for pattern matching switch with records
  • 905bcbe: 8286571: java source launcher from a minimal jdk image containing jdk.compiler fails with --enable-preview option
  • 8d28734: 8287741: Fix of JDK-8287107 (unused cgv1 freezer controller) was incomplete
  • f1dd559: 8287896: PropertiesTest.sh fail on msys2
  • 4fe0ca9: 8287860: Revise usage of volatile in j.u.Locale
  • bde7a7a: 8287236: Reorganize AST related to pattern matching for switch
  • 2d8c649: 8287663: Add a regression test for JDK-8287073
  • ... and 168 more: https://git.openjdk.java.net/jdk/compare/e9bddc18ab91c29d491b0e3bd145d641f6a62c5d...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jun 7, 2022
@openjdk openjdk bot closed this Jun 7, 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 Jun 7, 2022
@openjdk
Copy link

openjdk bot commented Jun 7, 2022

@aivanov-jdk @TejeshR13 Pushed as commit 67f1bd7.

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

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
7 participants