Closed
Bug 1015242
Opened 10 years ago
Closed 9 years ago
Provide a combined expected condition for element present and displayed
Categories
(Remote Protocol :: Marionette, defect)
Remote Protocol
Marionette
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla36
People
(Reporter: davehunt, Assigned: davehunt)
References
Details
(Whiteboard: [good first bug] [mentor=ato])
Attachments
(1 file, 1 obsolete file)
As discussed on IRC two common conditions that are commonly combined are element_present and element_displayed. Currently this is achieved without expected conditions as: Wait(m, ignored_exceptions=NoSuchElementException).until( lambda m: m.find_element(locator).is_displayed()) This has the disadvantage of repeatedly looking up the element even after it's been found and before it's displayed. It also doesn't take advantage of the expected conditions at all. To achieve this using the expected conditions we must do something like: Wait(m).until(expected.element_displayed( Wait(m).until(expected.element_present(locator)))) This is a little hard to read, and has the disadvantage of potentially doubling the timeout. We must first wait for the inner Wait to timeout before the outer Wait will do the same. Ideally we would be able to specify a single timeout. It would be great if we could have a single element_displayed expected condition that could take either a locator (and perform the lookup) or an element. The timeout for the Wait should apply to the entire condition, including the lookup if needed. Alternatively, a new condition such as element_present_and_displayed would also work.
Assignee | ||
Comment 1•10 years ago
|
||
Also, if this condition performs a lookup and returns the element, it means subsequent visibility conditions on this can avoid performing the lookup again. For example: element = Wait(m).until(expected.element_displayed(locator)) # do something that causes the element to be hidden Wait(m).until(expected.element_not_displayed(element))
Assignee | ||
Comment 2•10 years ago
|
||
Lastly, it would be great to do the same with element_not_displayed to allow for the situation where the element has not previously been looked up and may already be not present. An example might be a loading spinner or similar. For UI tests it's rare that we'll know if an element is hidden or removed entirely, so allowing element_not_displayed to take a locator and to return successfully if that locator does not find an element would be useful in some cases.
Assignee | ||
Comment 3•10 years ago
|
||
Andreas: Any thoughts on this? Could it be a mentored bug?
Flags: needinfo?(ato)
Comment 4•10 years ago
|
||
For element_present/element_not_present we allow the argument to be either a locator or an element reference. It makes sense to allow the same for element_displayed/element_not_displayed. It's also a good first bug.
Flags: needinfo?(ato)
Updated•10 years ago
|
Whiteboard: [good first bug] [mentor=ato]
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Andreas Tolfsen (:ato) from comment #4) > For element_present/element_not_present we allow the argument to be either a > locator or an element reference. It makes sense to allow the same for > element_displayed/element_not_displayed. > > It's also a good first bug. I just had a quick look into this, and have a couple of questions. I can successfully accept either a locator or an element for the displayed expected classes, however these currently only return the visibility of the element. In comment 1 I mentioned returning the element found, but I'm not sure how that would fit into the fix. Also, when expecting an element to be displayed we would want to first wait for it to be present, but when we're waiting for it to be not displayed we want to return early if the element is not present, but I'm not sure how we'd achieve this.
Flags: needinfo?(ato)
Assignee | ||
Comment 6•10 years ago
|
||
Here's a patch to see if I'm on the right path...
Comment 7•10 years ago
|
||
davehunt, this is a good first bug and you have a patch up here. do you need somebody to review or provide feedback on the patch?
Flags: needinfo?(dave.hunt)
Comment 8•10 years ago
|
||
You're on the right path. The API looks fine to me.
Flags: needinfo?(dave.hunt)
Flags: needinfo?(ato)
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Dave Hunt (:davehunt) from comment #5) > In comment 1 I mentioned returning the element found, but I'm not > sure how that would fit into the fix. I discussed this with Andreas, and we agreed that element_displayed and element_not_displayed should only return the visibility of the element. If a reference to the element is needed then the user should first locate the element either using find_element or element_present.
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8512619 [details] [diff] [review] bug1015242.patch I took another look over this patch and feel it's actually ready for review. Here's a try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=82f0f4c466ed
Attachment #8512619 -
Flags: feedback? → review?(ato)
Comment 11•9 years ago
|
||
Comment on attachment 8512619 [details] [diff] [review] bug1015242.patch Review of attachment 8512619 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: testing/marionette/client/marionette/expected.py @@ +167,5 @@ > + displayed = Wait(marionette).until(expected.element_displayed(By.ID, "foo")) > + > + Or by supplying an element:: > + > + el = Wait(marionette).until(expected.element_displayed(el)) Passing in el and returning to variable el is confusing. Can you provide an example that first looks up el and returns to the variable `displayed`, as the previous example does? @@ +184,3 @@ > > def __call__(self, marionette): > + if not self.el: if self.el is not None
Attachment #8512619 -
Flags: review?(ato) → review+
Assignee | ||
Comment 12•9 years ago
|
||
Hmm.. there was actually a few issues with the last patch. First was the docstrings as pointed out by :ato but also some logic was wrong, though I'm not sure why it was passing before. Here's a new try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c6397df2ad07
Attachment #8512619 -
Attachment is obsolete: true
Attachment #8529027 -
Flags: review?(ato)
Updated•9 years ago
|
Attachment #8529027 -
Flags: review?(ato) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a64ed135ea8
Keywords: checkin-needed
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4a64ed135ea8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•