Closed Bug 1275249 Opened 8 years ago Closed 8 years ago

Firefox-ui-tests permafail due to get_attributes() no longer returning property values

Categories

(Testing :: Firefox UI Tests, defect)

Product:
Testing
Component:
Firefox UI Tests
Version:
49 Branch
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(firefox48 fixed, firefox49 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: whimboo, Assigned: ato)

References

Details

(Keywords: intermittent-failure, regression)

Attachments

(1 file)

MozReview Request: Bug 1275249 - Update UI tests to get property where applicable; r?whimboo
58 bytes, text/x-review-board-request
whimboo
: review+
Details 
On mozilla-inbound our functional-ui-tests are perma broken because of the backward incompatible change on bug 1272653:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=Firefox%20UI&filter-tier=2&filter-tier=3

I wonder why we couldn't have left the fallback of using get_attribute() to return also values for properties, but mark it as deprecated.

This change will cause us issues with our update tests which happen across releases.
Flags: needinfo?(ato)
If it's a clear no-go to get fixed in a backward compatible manner in Marionette I have to check how to overlay that method from Marionette in our tests.
Yes, this is consciously a backwards breaking change in Marionette to align it with the WebDriver standard.  The idea is that WebDriver local ends that want the old behaviour can do this by using the Get Element Attribute and Get Element Property primitives to emulate it, like this:

    def get_attribute(attr):
        res = _send_message("getElementAttribute", {"value": attr}, key="value")
        if res is None:
            res = _send_message("getElementProperty", {"value": attr}, key="value")
        return res

In this particular case it looks like we need to replace four calls to get_attribute with get_property because they are fetching DOM properties in content.

XUL and chrome space should not see any breakage as a result of bug 1272653.
Flags: needinfo?(ato)
I talked with Andreas on IRC and we decided the following:

1) Andreas will get those broken test failures fixed.

2) We will wait how update tests are working. If those are failing we have to find a backward compatible method as mentioned above. Otherwise we should be good. At least the comment that only content is affected makes me less nervous ;)
In bug 1272653, Get Element Attribute was split into Get Element Attribute
for attributes and boolean attributes, and Get Element Property for
properties.

This patch addresses the fallout in the Firefox UI tests caused by
those patches.

Review commit: https://reviewboard.mozilla.org/r/54852/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54852/
Attachment #8755858 - Flags: review?(hskupin)
Assignee: nobody → ato
Status: NEW → ASSIGNED
Comment on attachment 8755858 [details]
MozReview Request: Bug 1275249 - Update UI tests to get property where applicable; r?whimboo

https://reviewboard.mozilla.org/r/54852/#review51492

Thanks Andreas! Lets get it landed so we have it on inbound before the next merge.
Attachment #8755858 - Flags: review?(hskupin) → review+
I will add the leave-open keyword for this bug because we have to check how update tests are working and if we need a further patch.
Assignee: ato → nobody
Status: ASSIGNED → NEW
Keywords: leave-open
Assignee: nobody → ato
Status: NEW → ASSIGNED
The fix was not complete for the functional tests. There is at least one more test which has not been reported as failure earlier?

TEST-UNEXPECTED-ERROR | test_security_notification.py TestSecurityNotification.test_invalid_cert | TypeError: argument of type 'NoneType' is not iterable

Andreas, can you provide a followup fix? If you do, please run the full suite to ensure its complete then.
Flags: needinfo?(ato)
Blocks: 1272228
Follow-up fix provided in bug 1275311.
Flags: needinfo?(ato)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
In case the update tests are broken I will file a new bug. So lets get the leave-opne keyword removed.
Keywords: leave-open
The patch on bug 1272653 got backported to 48.0, but this patch for the test never got to that branch! Lets get it into mozilla-beta.
status-firefox48: --- → affected
status-firefox49: affected → fixed
Whiteboard: [checkin-needed-beta]
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: