-
Notifications
You must be signed in to change notification settings - Fork 145
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
8159690: [TESTBUG] Mark headful tests with @key headful. #306
Conversation
Hi @yaqsun, welcome to this OpenJDK project and thanks for contributing! We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user yaqsun" as summary for the issue. If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing |
This backport pull request has now been updated with issue from the original commit. |
/covered |
Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated! |
We did start looking at backporting this a while back (see Paul's existing backport bug) Have you just omitted the 30 tests? We do need to check that these are not potential backports themselves before doing so, otherwise we risk the issue of bringing them in later and the What is the one test that doesn't need to handle Is there a particular reason you need this backported? I think it was eventually dropped because it's a lot of effort for not much gain. |
Yes, I omit 30 tests because they no longer exist in 8u. There is 1 test java/awt/image/DrawImage/IncorrectUnmanagedImageSourceOffset.java that does not need to handle @headful because @headful already exist. This backported will cause a large number of jdk/test tests to fail, interfering with regression testing. |
@yaqsun 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! |
I need help with progressing this pull request towards integration. |
@yaqsun 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! |
@yaqsun This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
/open |
@yaqsun This pull request is now open |
@yaqsun 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! |
/approval 8159690 request |
|
I think we should start from something, and then fix any missing part if any. |
I started by sorting fixes by test case name,eg:jdk/test/java/awt, javax/swing, sun/java2d, etc? |
I meant "something" - is the current patch on the review, we can integrate it as-is, and a few others currently under review. If we missed some tests then we can fix that later. |
Please maintainer approval my pr. |
I tend to agree, but we should at least know what follow-ups we want before this goes on, or they are likely to never happen. I'll try and look over this before the end of the week so we can get it in before rampdown. |
A reason should be given as to why the request should be approved. See https://wiki.openjdk.org/display/SKARA/Pull+Request+Commands#PullRequestCommands-/approval |
It's more that they never did, as they were introduced in 9.
Yes, I see this is covered by 8221497: Optional Panes in Swing
Ok. |
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.
Patch looks good. Copyright headers and context is adapted where needed.
Will add a list of the missed tests to this PR and their bugs. Some may be backportable, at least the test part even if we don't make the associated code change.
|
/approve yes |
@gnu-andrew |
@yaqsun This change now passes all automated pre-integration checks. 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 161 new commits pushed to the
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 (@mrserb, @gnu-andrew) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
/integrate |
8u422 open for commits |
Going to push as commit d7ad5bf.
Your commit was automatically rebased without conflicts. |
@gnu-andrew @yaqsun Pushed as commit d7ad5bf. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
The patch is not clean because the 31 tests are omitted, and because of various context differences. The only substantive change is the addition of @headful, which doesn’t affect running any of the tests.
There are 30 tests that were in the original patch that are not present in 8u. And there is 1 test that does not need to handle headful.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/306/head:pull/306
$ git checkout pull/306
Update a local copy of the PR:
$ git checkout pull/306
$ git pull https://git.openjdk.org/jdk8u-dev.git pull/306/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 306
View PR using the GUI difftool:
$ git pr show -t 306
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/306.diff
Webrev
Link to Webrev Comment