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

8272232: javax/swing/JTable/4275046/bug4275046.java failed with "Expected value in the cell: 'rededited' but found 'redEDITED'." #5079

Closed
wants to merge 3 commits into from

Conversation

prsadhuk
Copy link
Contributor

@prsadhuk prsadhuk commented Aug 11, 2021

The test fails in CI testing citing expected "rededited" but found "redEDITED", which seems to point to fact that either there is a CAPS_LOCK key switched on or some other test did not use SHIFT key corectly ie, pressed but not released.

Considering many more tests would have failed if CAPS_LOCK is turned on, I tried to find if some tests uses SHIFT key mistakenly and it seems JRadioButton test has press/release order wrong and it uses SHIFT. Rectified that.

In addition, corrected some known CI issues ie waiting after frame is made visible, made frame to centre. Along with that, also modified testcase to check lowercase string. Since the original JDK-4275046 issue is about newly entered text is made part of cell or not, it does not matter if it's in lowercase/uppercase and if such CAPS_LOCK or Shift key problem happens again, this test will not be affected.

CI run for several iterations running these 2 tests one after another in all platforms is green.


Progress

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

Issues

  • JDK-8272232: javax/swing/JTable/4275046/bug4275046.java failed with "Expected value in the cell: 'rededited' but found 'redEDITED'."
  • JDK-8257540: javax/swing/JFileChooser/8041694/bug8041694.java failed with "RuntimeException: The selected directory name is not the expected 'd ' but 'D '."

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5079

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

Using diff file

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

…ted value in the cell: 'rededited' but found 'redEDITED'.
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 11, 2021

👋 Welcome back psadhukhan! 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 label Aug 11, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 11, 2021

@prsadhuk The following label will be automatically applied to this pull request:

  • swing

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 swing label Aug 11, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 11, 2021

Webrevs

@mrserb
Copy link
Member

@mrserb mrserb commented Aug 12, 2021

I am not sure how it is possible that the shift was pressed while this test tried to type the text, the "redEDITED" mean that initially, the shift was unpressed? Looks like somebody login into the system and accidentally press that button, no?

@prsadhuk
Copy link
Contributor Author

@prsadhuk prsadhuk commented Aug 12, 2021

I am not sure of the actual reason...But I thought physically accessing the system at this time and pressing CAPS will be less possible at this time of pandemic than some test doing something wrong..
In Anycase, I have taken care of that in the test by checking lowercase so that if CAPS or shift is pressed by anyone/anytest, it willnot affect this test (and as I told original bug is about new characters being part of field, lowercsae/uppercase does not matter)

@mrserb
Copy link
Member

@mrserb mrserb commented Aug 12, 2021

Yes this cleanup looks fine, I just would like to highlight that the actual root cause of this issue is one of:

  • Someone presses the buttons while the test is executed
  • A few tests are executed in parallel(bug in jtreg or mach5?)
  • Bug in KVM which generates this button press.
    All of them are more important than the current cleanup, and I suggest investigating those possibilities on the lab_support+client_team chats

mrserb
mrserb approved these changes Aug 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 12, 2021

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

8272232: javax/swing/JTable/4275046/bug4275046.java failed with "Expected value in the cell: 'rededited' but found 'redEDITED'."
8257540: javax/swing/JFileChooser/8041694/bug8041694.java failed with "RuntimeException: The selected directory name is not the expected 'd ' but 'D '."

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

  • 14a3ac0: 8271911: replay compilations of methods which use JSR292 (easy cases)
  • d414a88: 8273240: Dynamic test ArchiveConsistency.java should use CDSArchiveUtils
  • 23fa0dc: 8272905: Consolidate discovered lists processing
  • ff4018b: 8268148: unchecked warnings handle ? and ? extends Object differently
  • 8c37909: 8273234: extended 'for' with expression of type tvar causes the compiler to crash
  • 28ba78e: 8244675: assert(IncrementalInline || (_late_inlines.length() == 0 && !has_mh_late_inlines()))
  • d05494f: 8266239: Some duplicated javac command-line options have repeated effect
  • 93eec9a: 8272776: NullPointerException not reported
  • 7b023a3: 8273257: jshell doesn't compile a sealed hierarchy with a sealed interface and a non-sealed leaf
  • f17ee0c: 8273263: Incorrect recovery attribution of record component type when j.l.Record is unavailable
  • ... and 217 more: https://git.openjdk.java.net/jdk/compare/5350b9901c6cebe5d40bbba9a31d1f26285b1cd6...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.

➡️ 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 label Aug 12, 2021
@prrace
Copy link
Contributor

@prrace prrace commented Aug 14, 2021

Someone presses the buttons while the test is executed
A few tests are executed in parallel(bug in jtreg or mach5?)
Bug in KVM which generates this button press.
All of them are more important than the current cleanup, and I suggest investigating those possibilities on the lab_support+client_team chats

I don't believe any of the scenarios.

As you well know these headful tests will be re-run 2 more times if they fail.

It seems improbable some one was sitting there waiting to jump on the keyboard and interfere

with the test at the exact moment 3 different times.

And the same for a KVM error.

As for in parallel we'd have hundreds of failures in such a case and we do not see that

And looking at the test log for when this failed 4 tests in total failed and they all have similar problems

===

java\awt\SplashScreen\MultiResolutionSplash\MultiResolutionSplashTest.java

Execution failed: `main' threw exception: java.lang.RuntimeException: Focus is lost! Expected 'ab' got 'AB'.


java/awt/Toolkit/RealSync/Test.java

Test failed: testType
[2021-08-10T04:06:59,103Z] Cause: Expected 'a' got 'A'.


javax/swing/JFileChooser/8041694/bug8041694.java

java.lang.RuntimeException: The selected directory name is not the expected 'd ' but 'D '.

======

Moreover this test isn't even the first that failed - that was MultiResolutionSplashTest - so the problem was
already there

So some test left SHIFT pressed. I can imagine a timing problem where a test was exited before it managed to release.

I am curious how Prasanta decided test/jdk/javax/swing/JRadioButton/8033699/bug8033699.java might be the problem

It was executed long after these tests were already failing.

So the root cause test is not yet identified.

@mrserb
Copy link
Member

@mrserb mrserb commented Aug 14, 2021

I don't believe any of the scenarios.

As you well know these headful tests will be re-run 2 more times if they fail.

But we know the log information only for the last failure, probably the button was pressed only for the last rerun. And according to this log, the shift or capslock was pressed during the test case execution. How it could be?

@prrace
Copy link
Contributor

@prrace prrace commented Aug 14, 2021

I don't believe any of the scenarios.
As you well know these headful tests will be re-run 2 more times if they fail.

But we know the log information only for the last failure, probably the button was pressed only for the last rerun. And according to this log, the shift or capslock was pressed during the test case execution. How it could be?

Not true ! stdout has everything ! All runs.Just a summary for a pass but all the data for a failure In one contoguous log file so there is no question.

@mrserb
Copy link
Member

@mrserb mrserb commented Aug 15, 2021

Not true ! stdout has everything ! All runs.Just a summary for a pass but all the data for a failure In one contoguous log file so there is no question.

Ok, so now we know that: according to this "contiguous log", the shift or capslock was pressed during the test case execution a few times in a row. How it could be?"

@mrserb
Copy link
Member

@mrserb mrserb commented Aug 15, 2021

Actually, the answer is simple. The test edits the cell in the table, the initial text in that cell is "red", the test appended the "edited" text. Since the capslock was enabled the actual result became "redEDITED".

@prsadhuk
Copy link
Contributor Author

@prsadhuk prsadhuk commented Aug 16, 2021

am curious how Prasanta decided test/jdk/javax/swing/JRadioButton/8033699/bug8033699.java might be the problem

As I told in initial explanation, I thought it will be either CAPS_LOCK key switched on or some other test did not use SHIFT key corectly ie, pressed but not released. I could not do anything about if someone press CAPS_LOCK so I search for VK_SHIFT being used in all javax/swing reg test and I found this test bug8033699.java is having a wrong order. I am not saying this was the root cause but it needs rectifying, I presume..
Also, the fix I proposed will not have any impact on this test testing 4275046 fix because of the reason mentioned above (original bug is about new characters being part of field, lowercsae/uppercase does not matter)

@prsadhuk
Copy link
Contributor Author

@prsadhuk prsadhuk commented Aug 16, 2021

There was one fix done way back regarding almost same tests being failed due to CAPS_LOCK not turned off by test
51837b8
but I could not find any other test doing something similar with setLockingState...
One was in java/awt/Toolkit/Headless/HeadlessToolkit.java but it is marked headless so would not affect headful test so I was thinking it's more of VK_SHIFT issue

@aivanov-jdk
Copy link
Member

@aivanov-jdk aivanov-jdk commented Aug 16, 2021

As I told in initial explanation, I thought it will be either CAPS_LOCK key switched on or some other test did not use SHIFT key corectly ie, pressed but not released.

This seems the likely cause. And in this case we have to find the test which leaves Shift presses or uses Caps Lock.

I could not do anything about if someone press CAPS_LOCK so I search for VK_SHIFT being used in all javax/swing reg test and I found this test bug8033699.java is having a wrong order. I am not saying this was the root cause but it needs rectifying, I presume..

In this test, bug8033699.java, the Shift key is released, however, it's released before Tab is released whereas (modifier) keys are usually released in the reverse order to the order they are pressed.

Have you already submitted a bug to fix the release order?

Also, the fix I proposed will not have any impact on this test testing 4275046 fix because of the reason mentioned above (original bug is about new characters being part of field, lowercsae/uppercase does not matter)

Yes, you're right the test is about getting the new text committed into the table cell. Yet the test didn't press Shift key, nor did it turn Caps Lock on, which means the problem is external to the test itself. If Shift key remains pressed, some other test could fails, for example, bug8033699.java will liked fail if it's started with Shift key press because focus will be traversed in the reverse order rather than forward order.

@@ -175,6 +177,7 @@ private void checkResult() throws Exception {
public void run() {
// Read the edited value of from the cell
editedValue = table.getModel().getValueAt(0, 1);
editedValue = ((String)editedValue).toLowerCase();
Copy link
Contributor

@prrace prrace Aug 16, 2021

Choose a reason for hiding this comment

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

So this just masks that some other test is the real problem.

@prrace
Copy link
Contributor

@prrace prrace commented Aug 16, 2021

I think what you need to do is take the list of tests that were run before the FIRST failure - not this one.
Keep them in order - very important
Now find all of them those that use SHIFT (more likey) or CAPS_LOCK
It should be at least easy to get that candidate list

Then work (backwards) through those to see which might have let SHIFT down which may be harder.

One thing I don't know (or understand) is why some other test that presses SHIFT did not release it and solve the problem.

Oh, and finding prior tests which would have failed if SHIFT was down but "pass" might also help the search.

@prsadhuk
Copy link
Contributor Author

@prsadhuk prsadhuk commented Aug 17, 2021

Since FIRST failure is java/awt/SplashScreen/MultiResolutionSplash/MultiResolutionSplashTest.java
I tried to find tests that uses VK_SHIFT key which are

java/awt/Debug/DumpOnKey/DumpOnKey.java
	java/awt/Dialog/NestedDialogs/Modal/NestedModalDialogTest.java
	java/awt/Dialog/NestedDialogs/Modeless/NestedModelessDialogTest.java
	java/awt/dnd/RecognizedActionTest/RecognizedActionTest.java
	java/awt/event/KeyEvent/8020209/bug8020209.java
	java/awt/event/KeyEvent/ExtendedModifiersTest/ExtendedModifiersTest.java
	java/awt/event/KeyEvent/KeyMaskTest/KeyMaskTest.java
	java/awt/event/KeyEvent/SwallowKeyEvents/SwallowKeyEvents.java
	java/awt/event/MouseEvent/MouseButtonsAndKeyMasksTest/MouseButtonsAndKeyMasksTest.java
	java/awt/FileDialog/MacOSGoToFolderCrash.java
	java/awt/Focus/8073453/AWTFocusTransitionTest.java
	java/awt/Focus/8073453/SwingFocusTransitionTest.java
	java/awt/Focus/FocusTraversalPolicy/ButtonGroupLayoutTraversal/ButtonGroupLayoutTraversalTest.java
	java/awt/FullScreen/AltTabCrashTest/AltTabCrashTest.java
	java/awt/keyboard/AllKeyCode/AllKeyCode.java
	java/awt/List/ActionEventTest/ActionEventTest.java
	java/awt/Mouse/MouseModifiersUnitTest/MouseModifiersUnitTest_Extra.java
	java/awt/Mouse/MouseModifiersUnitTest/MouseModifiersUnitTest_Standard.java
	java/awt/Robot/ModifierRobotKey/ModifierRobotEnhancedKeyTest.java
	java/awt/Robot/ModifierRobotKey/ModifierRobotKeyTest.java

and CAPS_LOCK keys


java/awt/keyboard/AllKeyCode/AllKeyCode.java
	java/awt/Toolkit/Headless/HeadlessToolkit.java
	java/awt/Toolkit/LockingKeyStateTest/LockingKeyStateTest.java

I could not find any misuse of VK_SHIFT apart from in
test/jdk/java/awt/List/ActionEventTest/ActionEventTest.java
test/jdk/java/awt/dnd/RecognizedActionTest/RecognizedActionTest.java
which I rectified.

For CAPS_LOCK, LockingKeyState was already rectified as I mentioned in previous comment and
HeadlessToolkit is headless which will not affect headful tests

robot.keyRelease(KeyEvent.VK_CONTROL);
robot.keyRelease(KeyEvent.VK_SHIFT);
robot.keyRelease(KeyEvent.VK_ALT);
dispose();
throw new RuntimeException("Action Event modifiers are not"
+ " set correctly.");
Copy link
Member

@aivanov-jdk aivanov-jdk Aug 17, 2021

Choose a reason for hiding this comment

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

This exception should be thrown from the main thread. An action listener is called on EDT, thus the exception won't fail the test, will it?

And using robot to release keys from EDT as part of the clean-up doesn't seem right.

Copy link
Contributor Author

@prsadhuk prsadhuk Aug 17, 2021

Choose a reason for hiding this comment

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

And using robot to release keys from EDT as part of the clean-up doesn't seem right.

I am just rectifying the order of the key release..It was pressed in order ALT SHIFT CONTROL and the release was ALT SHIFT CONTROL.. which I rectified to be release in order CONTROL SHIFT ALT

if (selectedDir.getName().equals("d")) {
if (selectedDir.getName().toLowerCase().equals("d")) {
throw new RuntimeException(
"JFileChooser removed trailing spaces in the selected directory name. " +
"Expected 'd ' got '" + selectedDir.getName() + "'.");
} else if (!selectedDir.getName().equals("d ")) {
} else if (!selectedDir.getName().toLowerCase().equals("d ")) {
Copy link
Member

@aivanov-jdk aivanov-jdk Aug 17, 2021

Choose a reason for hiding this comment

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

I believe @prrace's comment also applies here: So this just masks that some other test is the real problem.

@aivanov-jdk
Copy link
Member

@aivanov-jdk aivanov-jdk commented Aug 17, 2021

Since FIRST failure is java/awt/SplashScreen/MultiResolutionSplash/MultiResolutionSplashTest.java
I tried to find tests that uses VK_SHIFT key which are …

Can you reproduce the failure locally if the tests are run in the same order?

@prsadhuk
Copy link
Contributor Author

@prsadhuk prsadhuk commented Aug 19, 2021

Since FIRST failure is java/awt/SplashScreen/MultiResolutionSplash/MultiResolutionSplashTest.java
I tried to find tests that uses VK_SHIFT key which are …

Can you reproduce the failure locally if the tests are run in the same order?

Yes and no. These tests fail in my local system but not with same error message.
It is known that some CI test failures is difficult to reproduce locally due to system configuration and environment configuration(via KVM) in CI is quite different from local personal laptop so timing matters.
Also, it seems that these failures are intermittent. I waited for few days but it has not happened since, so it 's difficult to pinpoint what test timing issue is causing that.

Also, I admit that the test changes are masking this timing issues, but the changes are not wrong. If the original test author had used toLowerCase() initially, I don't think anyone would have objected as the test just checks for new characters without any distinction between lowercase/uppercase and this change would prevent this swing tests from failing.

@aivanov-jdk
Copy link
Member

@aivanov-jdk aivanov-jdk commented Aug 19, 2021

Since FIRST failure is java/awt/SplashScreen/MultiResolutionSplash/MultiResolutionSplashTest.java
I tried to find tests that uses VK_SHIFT key which are …

Can you reproduce the failure locally if the tests are run in the same order?

Yes and no. These tests fail in my local system but not with same error message.
It is known that some CI test failures is difficult to reproduce locally due to system configuration and environment configuration(via KVM) in CI is quite different from local personal laptop so timing matters.

Yeah, I know. Local environment is usually faster and more responsive.

Also, it seems that these failures are intermittent. I waited for few days but it has not happened since, so it 's difficult to pinpoint what test timing issue is causing that.

Do you mean that none of these tests has failed recently?
Could it be that another test stopped failing and, as a consequence, this test doesn't fail either?

Also, I admit that the test changes are masking this timing issues, but the changes are not wrong. If the original test author had used toLowerCase() initially, I don't think anyone would have objected as the test just checks for new characters without any distinction between lowercase/uppercase and this change would prevent this swing tests from failing.

I don't see why the original author would've bothered with toLowerCase when Shift or Caps Lock is never pressed in the test.

The test doesn't fail if it's run outside the long running test suite, does it? This means the test is correct as it is written now, its failure is caused by another misbehaving test which most likely has left Shift key pressed.

As an experiment, is it possible to log the current state of modifier keys, i.e. Shift, Control, Alt, before the key events are sent to edited table cell? This would confirm our guess whether Shift key is left pressed by a test which ran before.

To be clear, I'm not objecting this change, however, it doesn't feel right to me. At the same time, it's the quickest way to make the test pass. I'm pretty sure most of the tests dealing with text input don't use toLowerCase when the actual and expected values are compared.

@prsadhuk
Copy link
Contributor Author

@prsadhuk prsadhuk commented Aug 20, 2021

As an experiment, is it possible to log the current state of modifier keys, i.e. Shift, Control, Alt, before the key events are sent to edited table cell? This would confirm our guess whether Shift key is left pressed by a test which ran before.

It seems Toolkit.getLockingKetState() can only be called for CAPS_LOCK, NUM_LOCK, SCROLL_LOCK etc and not for modifiers. So, modifier keys can only be obtained from InputEvent. I used my own event listener inside the MultiResolutionSplashTest to get shift key via isSHiftDown() and run whole java_awt test in CI and it is shown to be "false" in that test so it seems no shift key is pressed at least for this run.



---------configuration:(4/107)----------
Boot Layer
  add modules: java.desktop            
  add exports: java.desktop/sun.java2d ALL-UNNAMED


----------System.out:(3/102)----------
VK_CAPS_LOCK key state: false
VK_SHIFT pressed key state: false
VK_SHIFT released key state: false
----------System.err:(1/16)----------
STATUS:Passed.
----------rerun:(48/5388)*----------



Maybe someother thing or some other timing issues happened the time these test fails.

@prsadhuk
Copy link
Contributor Author

@prsadhuk prsadhuk commented Sep 4, 2021

/issue add 8257540

@openjdk
Copy link

@openjdk openjdk bot commented Sep 4, 2021

@prsadhuk
Adding additional issue to issue list: 8257540: javax/swing/JFileChooser/8041694/bug8041694.java failed with "RuntimeException: The selected directory name is not the expected 'd ' but 'D '.".

@prsadhuk
Copy link
Contributor Author

@prsadhuk prsadhuk commented Sep 4, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Sep 4, 2021

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

  • 14a3ac0: 8271911: replay compilations of methods which use JSR292 (easy cases)
  • d414a88: 8273240: Dynamic test ArchiveConsistency.java should use CDSArchiveUtils
  • 23fa0dc: 8272905: Consolidate discovered lists processing
  • ff4018b: 8268148: unchecked warnings handle ? and ? extends Object differently
  • 8c37909: 8273234: extended 'for' with expression of type tvar causes the compiler to crash
  • 28ba78e: 8244675: assert(IncrementalInline || (_late_inlines.length() == 0 && !has_mh_late_inlines()))
  • d05494f: 8266239: Some duplicated javac command-line options have repeated effect
  • 93eec9a: 8272776: NullPointerException not reported
  • 7b023a3: 8273257: jshell doesn't compile a sealed hierarchy with a sealed interface and a non-sealed leaf
  • f17ee0c: 8273263: Incorrect recovery attribution of record component type when j.l.Record is unavailable
  • ... and 217 more: https://git.openjdk.java.net/jdk/compare/5350b9901c6cebe5d40bbba9a31d1f26285b1cd6...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 4, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Sep 4, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 4, 2021

@prsadhuk Pushed as commit cec6c06.

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

@prsadhuk prsadhuk deleted the JDK-8272232 branch Sep 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated swing
4 participants