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

Remove deprecated APIs #806

Closed
asolntsev opened this Issue Sep 19, 2018 · 16 comments

Comments

Projects
None yet
8 participants
@asolntsev
Copy link
Contributor

asolntsev commented Sep 19, 2018

In upcoming major release Selenide 5.0.0, we want to delete deprecated and unused functionality from Selenide. We need to know your opinion. Please say in comments if you use some of settings/functions below.

I suggest to delete the following settings:

  • Configuraton.collectionsTimeout (let it be same as Configuraton.timeout).
  • Configuration.collectionsPollingInterval (let it be same as Configuration.pollingInterval).
  • Configiration.closeBrowserTimeoutMs (browser is closed anyway in a separate daemon thread).
  • Configuration.chromeSwitches (it has been deprecated for a while)
  • Configuration.captureJavascriptErrors (it doesn't work well anyway) - remove JavascriptErrorsCollector as well
  • Configuration.setValueChangeEvent (we introduced this setting just in case to avoid breaking changes. Now it's clear for me that it's not needed).

I also suggest to delete the following deprecated Conditions:

  • present
  • hasAttribute
  • hasValue
  • hasText
  • hasClass

UPD. @eichisanden @Vyachelav @SergeyPirogov
The community voted that these two features should not be removed because people do actually use them:

  1. Configuration.assertionMode (it seems to me that soft assertion is not really useful in Selenide, and nobody actually uses it).
  2. Configuration.holdBrowserOpen (it's only needed during local debug - but you can debug without a setting anyway)

@asolntsev asolntsev added this to the 5.0.0 milestone Sep 19, 2018

@cocorossello

This comment has been minimized.

Copy link

cocorossello commented Sep 19, 2018

We use collectionsTimeout and collectionsPollingInterval, but it's better to leave as you say. I just changed the deprecated present and we don't use any more settings. Seems reasonable.

@eichisanden

This comment has been minimized.

Copy link

eichisanden commented Sep 20, 2018

Please do not delete Configuration.holdBrowserOpen.
Because I want to use Chrome Devtools to investigate the cause of error during debugging.
Since it can not be investigated if the browser is closed.

@aleksandr-kotlyar

This comment has been minimized.

Copy link

aleksandr-kotlyar commented Sep 20, 2018

hasAttribute
hasValue
hasText
hasClass

Ok, but i hope that has(Condition) will remain. Will it?

@asolntsev

This comment has been minimized.

Copy link
Contributor

asolntsev commented Sep 20, 2018

Yes, has(Condition) will remain. Thought, I still don't recommend to use these is/has methods that return boolean. It's a bad practice to put IFs in your tests.

@asolntsev

This comment has been minimized.

Copy link
Contributor

asolntsev commented Sep 20, 2018

@eichisanden It seems to be the most popular. Let's move discussion to the separate issue: #807

Please answer there, why the standard error message with screenshot and html is not enough.

@asolntsev asolntsev self-assigned this Sep 20, 2018

@Vyachelav

This comment has been minimized.

Copy link

Vyachelav commented Sep 20, 2018

Please, won't delete Configuration.assertionMode, my colleagues and I use SoftAssertions, it's really useful and more comfortable to use it for us. More comfortable than testng soft asserts.

@BorisOsipov

This comment has been minimized.

Copy link
Member

BorisOsipov commented Sep 23, 2018

EDITED: Fixed link

What about this?
https://github.com/codeborne/selenide/blob/a8b19f9c849ec9fa07ad9cab6dbb2080d1d7d08d/src/main/java/com/codeborne/selenide/Browsers.java#L13

I think we can remove it. It is easy to fix if someone still use this.

@asolntsev

This comment has been minimized.

Copy link
Contributor

asolntsev commented Sep 25, 2018

@BorisOsipov You mean method addListener()? I guess people use it - for example, for logging.

I even use it myself in the Gmail example: https://github.com/selenide-examples/gmail/blob/master/test/org/selenide/examples/gmail/GmailTests.java

@SergeyPirogov

This comment has been minimized.

Copy link
Collaborator

SergeyPirogov commented Sep 25, 2018

Listener is also used for allure integration with Selenide

 SelenideLogger.addListener("AllureSelenide", new AllureSelenide());
@rosolko

This comment has been minimized.

Copy link
Collaborator

rosolko commented Sep 25, 2018

@SergeyPirogov No plan to remove listeners. We just investigate if it's possible to remove IE alias and leave only InternetExplorer

@asolntsev

This comment has been minimized.

Copy link
Contributor

asolntsev commented Sep 25, 2018

@rosolko @BorisOsipov Now it's easy to run tests with ie: -Dselenide.browser=ie.
If we remove this constant, it will be a bit harder: -Dselenide.browser="internet explorer";. Million of problems with commas and spaces guaranteed. Is this constant really so bad?

@rosolko

This comment has been minimized.

Copy link
Collaborator

rosolko commented Sep 25, 2018

@asolntsev This constant marked as deprecated. So we need to decide if really need it and sd result keep it or remove.

@aleksandr-kotlyar

This comment has been minimized.

Copy link

aleksandr-kotlyar commented Sep 25, 2018

@asolntsev , i think "ie" is useful in your case. Please, unmark it as deprecated. One row is not huge, it's useful more important.

@BorisOsipov

This comment has been minimized.

Copy link
Member

BorisOsipov commented Sep 25, 2018

Now it's easy to run tests with ie: -Dselenide.browser=ie.

Agree

@rosolko rosolko referenced this issue Sep 25, 2018

Merged

Configuration cleanup and refactoring. #812

0 of 3 tasks complete

rosolko added a commit that referenced this issue Sep 26, 2018

Configuration cleanup and refactoring. (#812)
Set chrome as default browser and close #811.
Do not maximize browser by default and close #810.
Set default browser size to 1366x768.
Style all configuration with camelCase format.
Remove deprecate from internet explorer IE alias related on feedback from #806.
@rosolko

This comment has been minimized.

Copy link
Collaborator

rosolko commented Sep 27, 2018

Remove only closeBrowserTimeoutMsconfiguration.
Move value directly to CloseDriverCommand ctor.

rosolko added a commit to rosolko/selenide that referenced this issue Sep 27, 2018

@rosolko rosolko referenced this issue Sep 28, 2018

Merged

Remove deprecates. Close #806 issue. #816

3 of 3 tasks complete

@rosolko rosolko closed this in #816 Sep 30, 2018

rosolko added a commit that referenced this issue Sep 30, 2018

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