-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[java] reuse the classes created by the WebDriverDecorator #14789 #14793
Conversation
PR Reviewer Guide 🔍(Review updated until commit 906a5b3)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 906a5b3 Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit 35a12b1
Suggestions up to commit d74ce74
|
CI Failure Feedback 🧐(Checks updated until commit 0cce0cc)
|
d74ce74
to
35a12b1
Compare
Persistent review updated to latest commit 35a12b1 |
35a12b1
to
0937e25
Compare
@mykola-mokhnach Can you please help review this PR? |
java/src/org/openqa/selenium/support/decorators/WebDriverDecorator.java
Outdated
Show resolved
Hide resolved
java/src/org/openqa/selenium/support/decorators/WebDriverDecorator.java
Outdated
Show resolved
Hide resolved
java/src/org/openqa/selenium/support/decorators/WebDriverDecorator.java
Outdated
Show resolved
Hide resolved
java/src/org/openqa/selenium/support/decorators/WebDriverDecorator.java
Outdated
Show resolved
Hide resolved
java/src/org/openqa/selenium/support/decorators/WebDriverDecorator.java
Outdated
Show resolved
Hide resolved
java/src/org/openqa/selenium/support/decorators/WebDriverDecorator.java
Outdated
Show resolved
Hide resolved
0937e25
to
906a5b3
Compare
Persistent review updated to latest commit 906a5b3 |
As soon as a static cache is used the tests break with for me unexplainable ClassCastExceptions. @mykola-mokhnach could you review the other changes, it now looks mutch cleaner. |
906a5b3
to
310986c
Compare
java/test/org/openqa/selenium/support/decorators/DecoratedWebDriverTest.java
Show resolved
Hide resolved
310986c
to
1243079
Compare
Eagerly awaiting this fix going in, has it stalled? |
I appreciate the ongoing effort by @joerg1985 and @mykola-mokhnach. Let me know when this is in a good shape and we can merge it. |
I my mind this is ready to get merged, @mykola-mokhnach do you agree? |
go for it if tests pass |
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.
Thank you, @joerg1985!
Thanks, @mykola-mokhnach for the review!
User description
Description
This PR will add a cache to the
WebDriverDecorator
and reuse the classes created.To enabled reusing the handler, the target object is injected to a new field when a proxy is created.
As soon as the handler must call a method, the target object is red from the field inside the proxy.
Motivation and Context
Each invocation of a decorated method did return a new class, this will end in a OutOfMemoryError as soon as a certain number of classes has been generated. See #14789
Types of changes
Checklist
PR Type
Bug fix, Enhancement, Tests
Description
WebDriverDecorator
to reuse proxy classes, preventing excessive class creation and potentialOutOfMemoryError
.Definition
andProxyFactory
classes to manage proxy creation and caching.HasTarget
interface to facilitate access to target objects within proxies.Changes walkthrough 📝
WebDriverDecorator.java
Implement caching and reuse of proxy classes in WebDriverDecorator
java/src/org/openqa/selenium/support/decorators/WebDriverDecorator.java
Definition
andProxyFactory
classes for managing proxies.HasTarget
interface for accessing target objects.DecoratedWebDriverTest.java
Add tests for proxy class reuse and instance correctness
java/test/org/openqa/selenium/support/decorators/DecoratedWebDriverTest.java