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

Discuss whether ScreenCaptureWebDriverEventListener is necessary #985

Open
btonokit opened this issue Aug 16, 2022 · 4 comments
Open

Discuss whether ScreenCaptureWebDriverEventListener is necessary #985

btonokit opened this issue Aug 16, 2022 · 4 comments

Comments

@btonokit
Copy link
Contributor

btonokit commented Aug 16, 2022

Description

Please write the problem in detail.
Please write based on facts as much as possible, and post links to references/Javadoc or stack traces.

Deprecated org.openqa.selenium.support.events.EventFiringWebDriver and org.openqa.selenium.support.events.WebDriverEventListener.
Along with this, we will discuss whether to delete the WebDriverEventListenerAdapter that implements WebDriverEventListener and the ScreenCaptureWebDriverEventListener that inherits it.
EventFiringWebDriver is used in TransactionTokenTest to use ScreenCaptureWebDriverEventListener.

Possible Solution

Please write ideas or candidates of solutions for the problem if you have.
If it is an example, please enter a description such as "For example".

Affect versions

Version in which the problem occurs. Current version if not known.
If there is a feature suggestion, please delete it.

  • 5.x.x.RELEASE

Issue links

Links to related issues.

  • #xx
@yoshikawaa
Copy link
Contributor

ScreenCaptureWebDriverEventListener is not always necessary, but I think it's useful when debugging or troubleshooting.

@btkatoufm
Copy link
Contributor

btkatoufm commented Aug 17, 2022

Convenient for "useful when debugging or troubleshooting", you say. But to use this feature, I need to set up a listener and WebDriver with EventFiringWebDriver (EventFiringDecorator) and embed it in advance.

Of course, leaving this functionality in place might have the advantage of leaving it in a suspicious place to begin with, and then turning the flag on when you want to check and take a capture. However, the only case I have ever wanted to refer to a capture is when a test fails, and FunctionTestSupport#failedEvidence is sufficient.

If there is a case in the future where we want to take a capture at an arbitrary time, we can call ScreenCapture#saveForced directly, I think.

In any case, deprecation means that we will have to recreate it, but we can create a new one when it becomes necessary.

@yoshikawaa
Copy link
Contributor

failedEvidence saves the capture when the test fails, but the removed listener captures the progress of the screen transition. You may not feel much benefit if you maintain the test without changing it, but in tests that involve screen transitions, the cause of failure is often in the middle of the process. Incorporating saveForced into test logic each time is not a very enjoyable task. Also, preparing various means including listeners will be a hint for test implementation and reduce the burden on engineers. It's good to delete unnecessary things according to strict rules, but reducing the burden on engineers also leads to productivity. Of course deprecations need to be lifted, but that's not difficult.
I respect your point of view, but I don't agree with your motivation for the deletion. Thanks.

@btkatoufm
Copy link
Contributor

It was decided as follows.

  • Prepare listeners with incompatibilities resolved.
  • Fix to be valid not only for TransactionTokenTest but also for other tests.

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

No branches or pull requests

4 participants