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

CODETOOLS-7902793: Fix IllegalMonitorStateException in JInternalFrameOperatorCloseTest #4

Closed
wants to merge 3 commits into from

Conversation

shurymury
Copy link
Collaborator

@shurymury shurymury commented Dec 3, 2020

What worked was to override setClosed(boolean) in JInternalFrame to not close the frame.

Also while inspecting the stack trace, I have discovered that the description is not passed down from JInternalFrameOperator.waitClosed()


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Referenced JBS issue must only be used for a single change
  • Change must be properly reviewed

Issue

  • CODETOOLS-7902793: Fix IllegalMonitorStateException in JInternalFrameOperatorCloseTest

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jemmy-v2 pull/4/head:pull/4
$ git checkout pull/4

@shurymury
Copy link
Collaborator Author

shurymury commented Dec 3, 2020

@akolarkunnu @amresh-sahu, can you take a look?

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 3, 2020

👋 Welcome back shurailine! 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 Dec 3, 2020

@shurymury This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

7902793: Fix IllegalMonitorStateException in JInternalFrameOperatorCloseTest

Reviewed-by: akolarkunnu

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 3, 2020
@mlbridge
Copy link

mlbridge bot commented Dec 3, 2020

Webrevs

@shurymury shurymury requested a review from akolarkunnu Dec 3, 2020
Copy link

@amresh-sahu amresh-sahu left a comment

Not able to run the test due to NPE.

When close event is triggered by calling internalFrameOper.close(); JInternalFrame.doDefaultCloseAction() is called(). This method fires close property change event. Because we are closing the internal frame so this frame is removed from the desktop pane and when it is removed then its parent also set to null in Container.remove() method call.

I tested JInternalFrame add/remove workflow with Oracle's InternalFrameDemo class and It seems me valid. NPE is not a part of Product Bug.

java.lang.NullPointerException
at org.netbeans.jemmy.ClassReference.(ClassReference.java:51)
at org.netbeans.jemmy.EventDispatcher.(EventDispatcher.java:87)
at org.netbeans.jemmy.operators.ComponentOperator.(ComponentOperator.java:195)
at org.netbeans.jemmy.operators.ContainerOperator.(ContainerOperator.java:71)
at org.netbeans.jemmy.operators.WindowOperator.(WindowOperator.java:70)
at org.netbeans.jemmy.util.DefaultVisualizer.makeVisible(DefaultVisualizer.java:211)
at org.netbeans.jemmy.operators.ComponentOperator.makeComponentVisible(ComponentOperator.java:887)
at org.netbeans.jemmy.operators.AbstractButtonOperator.push(AbstractButtonOperator.java:350)
at org.netbeans.jemmy.drivers.windows.DefaultInternalFrameDriver.requestClose(DefaultInternalFrameDriver.java:59)
at org.netbeans.jemmy.operators.JInternalFrameOperator.close(JInternalFrameOperator.java:581)
at org.netbeans.jemmy.operators.JInternalFrameOperatorCloseTest.testClose(JInternalFrameOperatorCloseTest.java:99)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132)
at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:599)
at org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174)
at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46)
at org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822)
at org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147)
at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128)

@shurymury
Copy link
Collaborator Author

shurymury commented Dec 8, 2020

Thank you, @amresh-sahu, for the analysis. Like we discussed, I can not reproduce this problem locally. But I think I have enough to work on this.

@shurymury
Copy link
Collaborator Author

shurymury commented Dec 8, 2020

The exception was actually coming from the finally block where the test attempts to close an internal frame which acts absolutely normally (with field done been set to true). That, probably, means that there is something wrong in InternalFrameOperator.close(). Which is not related to this test, as this test is only supposed to test that waiting happens if the internal windows is not closing.
I have therefore removed internal frame closing at the end. It was unnecessary in the first place, as the test disposes the container window anyway.
The problem of the NPE coming from internal frame closing happening because the internal frame does not have a parent, should still be resolved. @amresh-sahu, can you be so kind file a separate bug on that with all the details you can provide? If you can create a standalone scenario for that, it would be great.

@shurymury
Copy link
Collaborator Author

shurymury commented Dec 8, 2020

@amresh-sahu , to make sure, can you run the test with the latest changes?

@shurymury
Copy link
Collaborator Author

shurymury commented Dec 8, 2020

As we have discovered with @amresh-sahu , it does not work in his environment.

@shurymury shurymury changed the title CODETOOLS-7902793: Fix IllegalMonitorStateException in JInternalFrameOperatorCloseTest WIP: CODETOOLS-7902793: Fix IllegalMonitorStateException in JInternalFrameOperatorCloseTest Dec 8, 2020
@shurymury shurymury removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 8, 2020
@openjdk openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 8, 2020
@shurymury shurymury changed the title WIP: CODETOOLS-7902793: Fix IllegalMonitorStateException in JInternalFrameOperatorCloseTest CODETOOLS-7902793: Fix IllegalMonitorStateException in JInternalFrameOperatorCloseTest Dec 8, 2020
Copy link

@amresh-sahu amresh-sahu left a comment

Changes look fine and these are working as expected.

Copy link
Collaborator

@akolarkunnu akolarkunnu left a comment

Looks good

Copy link
Collaborator

@akolarkunnu akolarkunnu left a comment

Looks good

@shurymury
Copy link
Collaborator Author

shurymury commented Dec 15, 2020

/integrate

@openjdk openjdk bot closed this Dec 15, 2020
@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 Dec 15, 2020
@openjdk
Copy link

openjdk bot commented Dec 15, 2020

@shurymury Pushed as commit 94612e2.

💡 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
integrated Pull request has been integrated
3 participants