Skip to content

Remove waiting from ElementProxy methods#211

Merged
tlconnor merged 4 commits intomasterfrom
tcPreventWaiting
Sep 20, 2021
Merged

Remove waiting from ElementProxy methods#211
tlconnor merged 4 commits intomasterfrom
tcPreventWaiting

Conversation

@tlconnor
Copy link
Copy Markdown

@tlconnor tlconnor commented Sep 15, 2021

The ElementProxy class has a set of methods that query the state of the element:

visible?
hidden?
present?
absent?

These methods all include waiting, so instead of just querying the state they wait for the condition to be true.

This causes considerable performance issues due to developers not being aware of this behavior. Since we have other methods for waiting for elements to be visible / hidden / present / absent, these methods should not wait at all.

@tlconnor tlconnor force-pushed the tcPreventWaiting branch 3 times, most recently from 5899d05 to dceaaa4 Compare September 15, 2021 03:28
Copy link
Copy Markdown

@skapfer skapfer left a comment

Choose a reason for hiding this comment

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

I like this change, because predicate methods should not implicitly wait for stuff to happen.

Two comments though

Comment thread lib/ae_page_objects/element_proxy.rb Outdated

options_or_locator = args.pop
options = if options_or_locator.is_a?(Hash)
options_or_locator.dup.merge(wait: wait)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The .dup. is unnecessary here because Hash::merge returns a copy

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good catch. Fixed.

true
rescue ElementNotVisible
false
reload_element
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Will this not raise LoadElementFailed in element.rb line 129 if the element is not visible?

Similar for the other predicates. I think the test stub behaves different from the real Element.rb in element_proxy_test.rb

Copy link
Copy Markdown
Author

@tlconnor tlconnor Sep 15, 2021

Choose a reason for hiding this comment

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

The reload_element method doesn't care if the element is visible or not, it will only raise an exception if the element doesn't exist.

There are 3 possible cases:

  1. The element exists and is visible.
    The element will be loaded and the method will return true.

  2. The element exists and is not visible:
    The element will be loaded and the method will return false.

  3. The element does not exist:
    The constructor of Element will raise LoadElementFailed. This is caught in ElementProxy#reload_element, which will result in @loaded_element = nil. The visible? method would then return false.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

My bad, I missed the rescue block.

Tim Connor added 3 commits September 16, 2021 10:57
The `ElementProxy` class has a set of methods that query the state of
the element.

```
visible?
hidden?
present?
absent?
```

These methods all include waiting, so instead of just querying the state
they *wait* for the condition to be `true`.

This causes considerable performance issues due to developers not being
aware of this behavior. Since we have other methods for waiting for
elements to be visible / hidden / present / absent, these methods should
not wait at all.
Copy link
Copy Markdown

@skapfer skapfer left a comment

Choose a reason for hiding this comment

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

Looks good to me, hope we don't have too many tests to fix for this!

true
rescue ElementNotVisible
false
reload_element
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

My bad, I missed the rescue block.

@tlconnor tlconnor merged commit 72cc7f1 into master Sep 20, 2021
@tlconnor tlconnor deleted the tcPreventWaiting branch September 20, 2021 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants