-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[java] reuse the classes created by the WebDriverDecorator #14789 #14793
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
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
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 |
diemol
left a comment
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
WebDriverDecoratorand 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
WebDriverDecoratorto reuse proxy classes, preventing excessive class creation and potentialOutOfMemoryError.DefinitionandProxyFactoryclasses to manage proxy creation and caching.HasTargetinterface to facilitate access to target objects within proxies.Changes walkthrough 📝
WebDriverDecorator.java
Implement caching and reuse of proxy classes in WebDriverDecoratorjava/src/org/openqa/selenium/support/decorators/WebDriverDecorator.java
DefinitionandProxyFactoryclasses for managing proxies.HasTargetinterface for accessing target objects.DecoratedWebDriverTest.java
Add tests for proxy class reuse and instance correctnessjava/test/org/openqa/selenium/support/decorators/DecoratedWebDriverTest.java