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

Explicit #738

Merged
merged 2 commits into from
Oct 2, 2017
Merged

Explicit #738

merged 2 commits into from
Oct 2, 2017

Conversation

titusfortner
Copy link
Contributor

@titusfortner titusfortner commented Sep 27, 2017

Change list

  • Changes name of constant since it actually isn't being used implicitly
  • Removes the resetting of implicit waits

Types of changes

What types of changes are you proposing/introducing to Java client?
Put an x in the boxes that apply

  • No changes in production code.
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • [x ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Details

Implicit waits are currently entirely overridden. Setting implicit wait to 0.5 or 300 won't make any difference when locating elements.

I don't think we should remove the user's ability to set an implicit wait, even if it can have negative consequences for them.

This is a "breaking" change because implicit waits will now be respected, which can allow users to combine implicit and explicit in ways that are not ideal, or cause longer test runs if users are testing for the absence of an element. But I think them not being respected before was technically the unexpected behavior, and this is essentially a bug fix for that.

I think this should be released with the recognition that people might have timing issues and that they should be told not to set implicit waits if they want to keep previous behavior.

I'm trying to think about how overriding the implicit wait setting in the appium driver, storing the value and doing fancy things with it during location method could work... but all I come up with would remove options from the user that I think they should have.

Anyway, let's discuss. Feel free to use/modify this code as necessary in anything you end up doing..

@titusfortner
Copy link
Contributor Author

Oh, forgot to mention this is for #735

@TikhomirovSergey
Copy link
Contributor

TikhomirovSergey commented Sep 28, 2017

@titusfortner Thank you very much. I will review it soon.
I think we should warn users to not use driver..manage().timeouts().implicitlyWait() if they use page/screen objects.

@@ -73,9 +73,9 @@
private final HasSessionDetails hasSessionDetails;


public AppiumFieldDecorator(SearchContext context, long implicitlyWaitTimeOut,
public AppiumFieldDecorator(SearchContext context, long timeOut,
Copy link
Contributor

Choose a reason for hiding this comment

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

-> timeout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are 32 places that use timeOut vs none with timeout. This PR should keep it consistent, though a later commit to change all usages is probably appropriate.

Copy link
Member

Choose a reason for hiding this comment

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

Why can't we change all the instances?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can/should, just didn't want to deal with those changes in this PR/release. :-)

Copy link
Contributor

@TikhomirovSergey TikhomirovSergey left a comment

Choose a reason for hiding this comment

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

@titusfortner
It looks ok. But could you remove the changeImplicitlyWaitTimeOut? It seems to be unused.

@titusfortner
Copy link
Contributor Author

I think we should warn users to not use driver.manage().timeouts().implicitlyWait() if they use page/screen objects.

I'm not sure what difference it makes page objects vs non. Isn't this locator code what is used regardless?

That being said, I'm all about warning users against using implicit waits, is there a mechanism to do so, or are we just talking about in the responses we give in the likely github issues? :)

@TikhomirovSergey - I removed changeImplicitlyWaitTimeOut, so I think this should be good.

@SrinivasanTarget
Copy link
Member

That being said, I'm all about warning users against using implicit waits, is there a mechanism to do so, or are we just talking about in the responses we give in the likely github issues? :)

@titusfortner @TikhomirovSergey AFAIK Selenium is planning to deprecate implicit waits soon. Simon had spoke about deprecating it on last SeleniumConf. May be we can wait for it.

@TikhomirovSergey
Copy link
Contributor

@SrinivasanTarget ok. So there is nothing to worry about.

@titusfortner
Copy link
Contributor Author

@SrinivasanTarget Simon said not to use them, but he still put them in the w3c spec, so I don't think they'll be actually deprecated any time soon.

@TikhomirovSergey TikhomirovSergey mentioned this pull request Sep 29, 2017
4 tasks
@SrinivasanTarget
Copy link
Member

Simon said not to use them, but he still put them in the w3c spec, so I don't think they'll be actually deprecated any time soon.

Ok

@TikhomirovSergey TikhomirovSergey merged commit a971cc8 into appium:master Oct 2, 2017
TikhomirovSergey added a commit that referenced this pull request Oct 2, 2017
heeseon added a commit to heeseon/java-client that referenced this pull request Oct 15, 2017
…-client into readperformancedata

* 'readperformancedata' of https://github.com/heeseon/java-client: (156 commits)
  build error
  Do not hardcode
  Do not hardcode
  Update README.md
  Update README.md
  Code style issues which were found by reviewer were fixed
  Code style issues which were found by reviewer were fixed
  The addition to appium#738 - following dependencies were updated:   `org.seleniumhq.selenium:selenium-java` to 3.6.0   `com.google.code.gson:gson` to 2.8.2   `org.springframework:spring-context` to 5.0.0.RELEASE   `org.aspectj:aspectjweaver` to 1.8.11
  do not zero out implicit wait during location call
  rename DEFAULT_IMPLICITLY_WAIT_TIMEOUT to DEFAULT_TIMEOUT
  Update README.md
  some minor things that were found by reviewers were improved
  code style issues were got fixed
  appium#732 FIX
  ServerBuilderTest: ip calculation was improved
  ServerBuilderTest: the path resolving
  ServerBuilderTest: magic strings were turned into final values
  ServerBuilderTest: magic strings were turned into final values
  ServerBuilderTest: code improvement.
  Tests of local appium DriverService were re-designed.
  ...
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.

4 participants