8051591: Test javax/swing/JTabbedPane/8007563/Test8007563.java fails #20791
Conversation
|
👋 Welcome back abhiscxk! A progress list of the required criteria for merging this PR into |
|
@kumarabhi006 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: 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 294 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. ➡️ To integrate this PR with the above commit message to the |
|
@kumarabhi006 The following label will be automatically applied to this pull request:
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. |
Webrevs
|
There was a problem hiding this comment.
SecondaryLoop secondaryLoop =
Toolkit.getDefaultToolkit().getSystemEventQueue()
.createSecondaryLoop();
new Thread() {
@Override
public void run() {
try {
Thread.sleep(200);
} catch (InterruptedException e) {
}
secondaryLoop.exit();
invokeLater(Test8007563.this);
}
}.start();
secondaryLoop.enter();
I'm not sure if the SecondaryLoop code is required for this test or it can be simplified?
I have tried simplifying the test code and removed SecondaryLoop code. Tested the current changes in CI across all platforms and no failure observed. Posted the CI link in JBS. |
|
@prsadhuk As we were discussing about the test change and removal of SecondaryLoop change, I tested the current changes without the fix proposed in JDK-8007563 and test failed which is correct. After applying the fix, test is able to pass. |
|
@prsadhuk Ran the test with uiscale of 2 and test passed across all platforms. |
Secondary loop was added for JDK-8078269 not for JDK-8007563 so you need to check if test fail without Secondary loop without that fix and passes with the fix.. |
I tested without the fix for JDK-8078269 for different scenarion:
Since the old test with longer delay (500 ms) and changed test with delay of 500 ms passed without the fix of JDK-8078269, looks like the reason for failure could be less delay. |
|
I am not sure why secondary loop was added in the testcase for JDK-8078269.. |
That is unclear to me as well.
This issue still exists with the existing implementation in MetalTabbedPaneUI. If we add I think removal of SecondaryLoop should be ok for this PR. And for JDK-8078269 as you suggested we may need a new test case which checks for the |
As per
I guess because pre-2018 our tests was run in samevm mode so there might be some stability issue and since it has been changed to othervm mode by default since '18, I guess this loop is not needed. but if background Color set by user as is in this case in the testcase, it will not be instance of UIResource but Color so background color will take precedence |
There was a problem hiding this comment.
I tested this myself and it does seem to be stable and pass consistently. Looks like the extra delay works.
Following the logic regarding the precedence of the different sources of background color, it does seem like Prasanta is correct when glancing over the different sources of colors.
Worth noting, I think it'd be good to rename the test since we're removing it from the problemlist and moving the test to a new location anyway. Maybe rename it to JTabbedPaneBackgroundColorTest or something similar to be more descriptive.
I didn't pay attention that the file is recognized as a new file when it has moved to new location. This may be due to the recent changes in test where it has been almost changed except few lines and is recognized as new file. |
|
Seems like closed test 4690946 where the UI property "TabbedPane.tabAreaBackground" was added in webrev.0 is moved to open and renamed as javax/swing/JTabbedPane/TestJTabbedPaneOpaqueColor.java..Any idea why the test is only for windows and linux? Can it be extended for macos too? Is JDK-7148101 still valid? |
closed test 4690946 was moved to open for the issue JDK-8226990 and was tested against the windows and linux platform only because the issue was reported for Nimbus and GTK L&F. See the discussion here.
Do you mean TestJTabbedPaneOpaqueColor Test ? I guess it can be extended for Mac but we need to restrict the testing only for Metal and Nimbus L&F on Mac machine. |
Yes, issue still exists for mac machine. |
Issue is for AquaL&F which as per your verification in JDK-8331619 and #19170 is not an issue it seems.. |
Yes, for Aqua L&F we were not able to verify the native behavior. |
Yes already raised the PR for removal.. |
| */ | ||
|
|
||
| public class TestJTabbedPaneBackgroundColor { | ||
| private static final ArrayList<String> LIST = new ArrayList<>(); |
There was a problem hiding this comment.
I think it can be changed to a non-final static var
| private static final ArrayList<String> LIST = new ArrayList<>(); | |
| private static ArrayList<String> lafList = new ArrayList<>(); |
|
|
||
| public static void main(String[] args) throws Exception { | ||
| robot = new Robot(); | ||
| robot.setAutoDelay(200); |
There was a problem hiding this comment.
This can be removed as there no Robot mouse or key events.
robot.setAutoDelay(200);
|
@prsadhuk Minor update after you reviewed. Please re-review the PR. |
There was a problem hiding this comment.
LGTM.
Suggestion:: Adding a consolidated note listing the JBS ids & corresponding tests related to JTabbedPane background issue in this PR or JBS might be helpful for future reference (avoids scrolling through PR comments).
- JDK-8051591 - Test javax/swing/JTabbedPane/8007563/Test8007563.java => test/jdk/javax/swing/JTabbedPane/TestJTabbedPaneBackgroundColor.java
...
so on.
|
@kumarabhi006 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! |
|
/integrate |
|
Going to push as commit 780de00.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@kumarabhi006 Pushed as commit 780de00. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Test was failing on multiple platforms mainly windows and linux and was problemlisted for all platforms. Increased the delay from 200ms to 500 ms to fetch the correct background color using robot. Now test passed for multiple iteration (~50 times) in CI pipeline for all platforms. Wild imports are expanded and test is pulled out of an extra level of folder.
Test removed from
Problem-listfile as well.CI testing link is posted in JBS.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20791/head:pull/20791$ git checkout pull/20791Update a local copy of the PR:
$ git checkout pull/20791$ git pull https://git.openjdk.org/jdk.git pull/20791/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 20791View PR using the GUI difftool:
$ git pr show -t 20791Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20791.diff
Webrev
Link to Webrev Comment