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

[java] remove deprecated classes and their tests #13200

Merged

Conversation

LogicOscar
Copy link
Contributor

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

remove deprecated event classes. extension of #12578

Description

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@titusfortner
Copy link
Member

I'm going to merge #13198 & #13199 for 4.16.
I think we merge https://github.com/SeleniumHQ/seleniumhq.github.io/pull/1533/files
And then merge this one with 4.17?

@diemol what do you think?

@titusfortner titusfortner added this to the 4.17 milestone Nov 27, 2023
@diemol
Copy link
Member

diemol commented Dec 5, 2023

I'm going to merge #13198 & #13199 for 4.16. I think we merge https://github.com/SeleniumHQ/seleniumhq.github.io/pull/1533/files And then merge this one with 4.17?

@diemol what do you think?

Yes. Good idea.

Thank you, @RevealOscar!

Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

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

Thank you, @RevealOscar!

@diemol diemol merged commit 259a94d into SeleniumHQ:trunk Dec 8, 2023
13 checks passed
diemol pushed a commit to SeleniumHQ/seleniumhq.github.io that referenced this pull request Dec 8, 2023
* add mini blog

* Update website_and_docs/content/blog/2023/java-removal-of-deprecated-events-classes.md

---------

Co-authored-by: Diego Molina <[email protected]>

Merging as SeleniumHQ/selenium#13200 got merged.

[deploy site]
selenium-ci added a commit to SeleniumHQ/seleniumhq.github.io that referenced this pull request Dec 8, 2023
* add mini blog

* Update website_and_docs/content/blog/2023/java-removal-of-deprecated-events-classes.md

---------

Co-authored-by: Diego Molina <[email protected]>

Merging as SeleniumHQ/selenium#13200 got merged.

[deploy site] 26d7cfd
@valfirst
Copy link
Contributor

@diemol I think it was too early to merge this PR. #13210 is not completed, it closes the gap and provide the full replacement for the functionality removed by this PR.

IMHO the expected flow should be:

The functionality is removed from trunk, but no replacement is available at the current moment.

@titusfortner
Copy link
Member

titusfortner commented Dec 11, 2023

This PR is not in a production release. It is in preparation for 4.17 which will be released at the beginning of the year.

What we have here is 95+% solution and addresses 99+% of use cases from existing users.

You can always create your own WebDriverDecorator if you need something that is missing.

That said, you're right, there are more things we need to do in #13210, and we want to get that out in 4.17 as well. @RevealOscar how is that PR going? Do you have time to work on that in the next few weeks? Do you want help finishing it up?

@valfirst
Copy link
Contributor

This PR is not in a production release. It is in preparation for 4.17 which will be released at the beginning of the year.

Yes, this is clear. My comment is aimed at potentially minimizing risks. For now, we've removed the deprecated functionality. However, the replacement is not ready yet and hasn't undergone testing since we don't fully comprehend all the use cases; we can merely conjecture. This sets up the risk of losing certain functionalities in the next release, or releasing functionality that may not meet user needs. In other words, users may face difficulties migrating to newer versions.

What we have here is 95+% solution and addresses 99+% of use cases from existing users.

Let's be honest, we don't know usage statistics 😄

You can always create your own WebDriverDecorator if you need something that is missing.

Certainly, it's possible. However, the issue is that users would need to implement workarounds for the reworked functionalities that are not yet usable. In essence, the rework would not present any value, only result in losses.

My proposal:

If the team lacks of resources and time, I can help with implementation of #13210.

@diemol
Copy link
Member

diemol commented Dec 11, 2023

@valfirst

Let's be honest, we don't know usage statistics 😄

This might be true. However, most people do not move to a new implementation until the deprecated code is no longer there. We have seen this when we released Selenium 4 and other major versions.

So yeah, we could have both sets of code, even a fully completed and featured new listener, but that does not mean people move to it. The old one has been deprecated for a long time, and I believe you have been the only one raising concerns.

I would say we focus on finishing #13210 and being reactive to issue reports.

@titusfortner
Copy link
Member

I'm on record that I think all of this code should be removed from Selenium entirely and implemented by someone else in a separate package.

The only reason I'm spending any time reviewing this at all is to support deleting deprecated code.

If you can find a single reference anywhere online to someone using a Listener method that isn't supported in the current implementation, I'll happily provide a public mea-culpa. But I've removed support for many features that had more users than this, so I'm more than happy to upset the one person who may be using this. 😂

Let's see where Oscar is at in availability before we jump in with additional help.

@valfirst
Copy link
Contributor

I use Listener methods which are not supported yet, I can't move to new implementation and need to live with deprecated API, that's why I'm so active in this topic 😉 :
https://github.com/vividus-framework/vividus/blob/74133cc9654dbe213801e7d8bfaac236dc432b40/vividus-plugin-web-app/src/main/java/org/vividus/ui/web/listener/WebUiContextListener.java#L57-L77.

@diemol
Copy link
Member

diemol commented Dec 12, 2023

Can you add the missing methods to #13210? Either as a comment or as a code example.

@LogicOscar
Copy link
Contributor Author

@diemol @valfirst @titusfortner I don't mind adding or having someone else add the target locator stuff thats missing to #13210
I can set some time aside hopefully today but most likely within the next 2 days. Work has been stretching me thin with transitioning me into a sre position and still doing my regular sdet things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants