Closed
Bug 1095303
Opened 10 years ago
Closed 10 years ago
Loop functional e2e test is broken
Categories
(Hello (Loop) :: General, defect, P1)
Hello (Loop)
General
Tracking
(firefox34 wontfix, firefox35 wontfix, firefox36 fixed, firefox37 fixed)
backlog | Fx37+ |
People
(Reporter: drno, Assigned: drno)
References
Details
(Whiteboard: [tech-debt])
Attachments
(1 file, 7 obsolete files)
8.59 KB,
patch
|
drno
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
I noticed today that the functional e2e Loop test:
./mach marionette-test browser/components/loop/test/functional/
is broken. It fails like this:
0:21.76 TEST_END: MainThread ERROR, expected PASS
Traceback (most recent call last):
File "/Users/nohlmeier/src/mozilla-central/testing/marionette/client/marionette/marionette_test.py", line 267, in run
testMethod()
File "/Users/nohlmeier/src/mozilla-central/browser/components/loop/test/functional/test_1_browser_call.py", line 150, in test_1_browser_call
self.switch_to_panel()
File "/Users/nohlmeier/src/mozilla-central/browser/components/loop/test/functional/test_1_browser_call.py", line 60, in switch_to_panel
self.marionette.switch_to_frame(frame)
File "/Users/nohlmeier/src/mozilla-central/testing/marionette/client/marionette/marionette.py", line 1002, in switch_to_frame
response = self._send_message('switchToFrame', 'ok', element=frame.id, focus=focus)
File "/Users/nohlmeier/src/mozilla-central/testing/marionette/client/marionette/decorators.py", line 36, in _
return func(*args, **kwargs)
File "/Users/nohlmeier/src/mozilla-central/testing/marionette/client/marionette/marionette.py", line 638, in _send_message
self._handle_error(response)
File "/Users/nohlmeier/src/mozilla-central/testing/marionette/client/marionette/marionette.py", line 674, in _handle_error
raise errors.NoSuchFrameException(message=message, status=status, stacktrace=stacktrace)
NoSuchFrameException: NoSuchFrameException: Unable to locate frame: undefined
According to Dan it was working 2 days ago.
As the Loop iframe is still called 'loop' and the find_element() actually returns valid looking data, something seems to go wrong in switch_to_frame(). Maybe a marionette bug which got introduce in the last 2 days since Dan ran this last successfully?
Assignee | ||
Comment 1•10 years ago
|
||
I rolled marionette back to changeset 213081 from Oct 29. The problem persists.
Is this test also failing for Aurora/Beta builds?
Flags: needinfo?(drno)
Assignee | ||
Comment 3•10 years ago
|
||
This fixes python errors on the Aurora branch.
Flags: needinfo?(drno)
Assignee | ||
Comment 4•10 years ago
|
||
After applying the patch I just attached the test runs on Aurora into exactly the same error as on central.
(In reply to Nils Ohlmeier [:drno] from comment #4)
> After applying the patch I just attached the test runs on Aurora into
> exactly the same error as on central.
So this bug does not impact Aurora/Beta in their current state?
Comment 6•10 years ago
|
||
So Marionette changed its API a bit recently, and your patch switches the test code back to the old Marionette API (which is what's on Aurora).
It _might_ still be a recent landing that is making the functional test fail in both places, given how many things we've been uplifting from Nightly to Aurora and beyond.
Comment 7•10 years ago
|
||
Which _seems_ like it suggests that the breakage is probably not in Marionette, unless Marionette patches have been uplifted in the last small number of days too.
I'd like to better understand what branches are affected. This is an absolute blocker as far as I'm concerned and I need to know which versions are affected so it can be tracked appropriately.
Comment 9•10 years ago
|
||
Can we get a regression range? And what exactly is breaking since users are clearly able to still use Hello without any reported issues? This sounds like a bug in the test and how it interacts with Marionette.
Updated•10 years ago
|
Keywords: regressionwindow-wanted
Assignee | ||
Comment 10•10 years ago
|
||
On Beta it is broken too because it fails to find loop-button-throttled, which is not surprising as the loop button is not shown in Beta. I guess the test would need to get patched to move the Loop button up into the UI first.
Comment 11•10 years ago
|
||
(In reply to Nils Ohlmeier [:drno] from comment #10)
> On Beta it is broken too because it fails to find loop-button-throttled,
> which is not surprising as the loop button is not shown in Beta. I guess the
> test would need to get patched to move the Loop button up into the UI first.
Correct, the test has always assumed that the loop button is there.
Assignee | ||
Comment 12•10 years ago
|
||
As this test never ran anywhere in an automated way or as part of any Ci system I don't see this as a blocker. It is more then annoying that it broke without anyone noticing.
A regression rage is not that easy to get as it would be a completely manual process of finding that (because of the external dependencies which also prevents us from running this on TBPL).
Comment 13•10 years ago
|
||
Thanks for helping me understand this. Since this wasn't running anywhere before I'm okay to let this slip if it must. That said, please make sure this is tracked as a P1 in the Trello dashboard, if it isn't already.
Updated•10 years ago
|
backlog: --- → Fx37+
Priority: -- → P1
Whiteboard: [tech-debt]
Assignee | ||
Comment 14•10 years ago
|
||
David, this is the original bug report that our test is broken. Could you attach your patch(es) here?
Flags: needinfo?(dburns)
Assignee | ||
Comment 15•10 years ago
|
||
David, I figured that the indentation of switchToFrame() is actually broken. This should put it back to saner levels.
Assignee | ||
Comment 16•10 years ago
|
||
Comment on attachment 8519177 [details] [diff] [review]
test_1_browser_call.aurora.patch
This patch is no longer needed, as the changes got uplifted to Aurora.
Attachment #8519177 -
Attachment is obsolete: true
Comment 17•10 years ago
|
||
I have found that bug 1107915 is the cause of the frame issue
Depends on: 1107915
Flags: needinfo?(dburns)
Comment 18•10 years ago
|
||
I have pushed a patch to try and for review and hopefully can land today
Comment 19•10 years ago
|
||
the next issue is on http://dxr.mozilla.org/mozilla-central/source/browser/components/loop/test/functional/test_1_browser_call.py#70
that is no longer an input it is a `button` element, changing it like that got me a bit further so I can make sure you are all on the right tryack
Assignee | ||
Comment 20•10 years ago
|
||
Thanks David. I verified that your fix gets me to the same problem now. Which is basically caused by the fact that the UI has changed since we ran the test last time successfully. I'm looking into updating the test...
Assignee: nobody → drno
Assignee | ||
Updated•10 years ago
|
Attachment #8532294 -
Attachment is obsolete: true
Assignee | ||
Comment 21•10 years ago
|
||
Just a patch to make the remaining work on this easier...
Assignee | ||
Comment 22•10 years ago
|
||
This skips the first time user tour and clicks the "Start a conversation" button.
Note: it has an ugly sleep(2) in it, because the UI seems to be too slow for the test.
The big problem now is: the link clicker URL is a text entry in the middle of a crap load of react generated stuff without any ID's or unique CSS classes which would allow us to select the text field.
Flags: needinfo?(dmose)
Assignee | ||
Comment 23•10 years ago
|
||
The one magic ICE setting to allow offline calls for this test.
Note: I did not remove the comment, because I'm not sure if we really need to do something else with the ICE servers. I don't think so, but it sounded like the author of the comment thought so.
Assignee | ||
Comment 24•10 years ago
|
||
Removing regression window as the marionette problem is fixed, and we now have to update our own test to our latest code.
Keywords: regressionwindow-wanted
Comment 25•10 years ago
|
||
Spent some time pairing with Nils, we've unblocked this particular problem, and are on to the next, which is making navigation to standalone page work, which Nils is looking into more.
Flags: needinfo?(dmose)
Assignee | ||
Comment 26•10 years ago
|
||
So it turns out that the rooms "obviously" have their own webAppUrl defined. By default that points to localhost:3000, but the test starts the standalone server at localhost:3001.
So after updating that I get the standalone page to load with the new rooms UI, but then it obviously hits the next problem as the UI has changed again.
0:27.32 TEST_END: MainThread ERROR, expected PASS
Traceback (most recent call last):
File "/Users/nohlmeier/src/mozilla-central/testing/marionette/client/marionette/marionette_test.py", line 267, in run
testMethod()
File "/Users/nohlmeier/src/mozilla-central/browser/components/loop/test/functional/test_1_browser_call.py", line 150, in test_1_browser_call
self.load_and_verify_standalone_ui(call_url)
File "/Users/nohlmeier/src/mozilla-central/browser/components/loop/test/functional/test_1_browser_call.py", line 63, in load_and_verify_standalone_ui
call_url_link = self.marionette.find_element(By.CLASS_NAME, "call-url") \
File "/Users/nohlmeier/src/mozilla-central/testing/marionette/client/marionette/marionette.py", line 1313, in find_element
response = self._send_message('findElement', 'value', **kwargs)
File "/Users/nohlmeier/src/mozilla-central/testing/marionette/client/marionette/decorators.py", line 36, in _
return func(*args, **kwargs)
File "/Users/nohlmeier/src/mozilla-central/testing/marionette/client/marionette/marionette.py", line 630, in _send_message
self._handle_error(response)
File "/Users/nohlmeier/src/mozilla-central/testing/marionette/client/marionette/marionette.py", line 664, in _handle_error
raise errors.NoSuchElementException(message=message, status=status, stacktrace=stacktrace)
NoSuchElementException: NoSuchElementException: Unable to locate element: call-url
Assignee | ||
Comment 27•10 years ago
|
||
I have a really hacked version of the test locally passing again for me!
Assignee | ||
Comment 28•10 years ago
|
||
This should fix the remaining issue for now.
Note: lot's of short cuts, which should be fixed before landing.
Comment 29•10 years ago
|
||
W00t! Nice work, Nils!
Assignee | ||
Comment 30•10 years ago
|
||
This combined patch makes the test work for me again.
I'm going to try it manually on the QA lab server now.
Attachment #8533907 -
Attachment is obsolete: true
Attachment #8533909 -
Attachment is obsolete: true
Attachment #8533911 -
Attachment is obsolete: true
Attachment #8534813 -
Attachment is obsolete: true
Attachment #8535302 -
Flags: review?(dmose)
Assignee | ||
Comment 31•10 years ago
|
||
The one magic settings to get this working with loop server and rooms in the loop server config:
"rooms": {
"webAppUrl": "http://localhost:3001/content/{token}",
...
}
Comment 32•10 years ago
|
||
Comment on attachment 8535302 [details] [diff] [review]
bug_1095303_update_to_rooms_ui.patch
Review of attachment 8535302 [details] [diff] [review]:
-----------------------------------------------------------------
W00t; it's wonderful to see this working again. Thanks for the patch! r=dmose with the two comments answered or addressed in code changes.
::: browser/components/loop/test/functional/config.py
@@ -7,5 @@
> - # Need to find the correct Pythonic syntax for this (unless just using
> - # a string is the way to go), as well as find the bug with the
> - # other preference for shunting ice to localhost that ekr/drno did,
> - # and other stuff before we can support offline call tests
> - # XXX "media.peerconnection.default_iceservers": "[]",
I'm surprised it makes sense for this to go away. Aren't we going to need it?
::: browser/components/loop/test/functional/test_1_browser_call.py
@@ +124,5 @@
>
> def test_1_browser_call(self):
> self.switch_to_panel()
>
> + #foo = self.start_a_conversation()
This can be deleted, yes?
Attachment #8535302 -
Flags: review?(dmose) → review+
Comment 33•10 years ago
|
||
(In reply to Nils Ohlmeier [:drno] from comment #31)
> The one magic settings to get this working with loop server and rooms in the
> loop server config:
>
> "rooms": {
> "webAppUrl": "http://localhost:3001/content/{token}",
> ...
> }
You should be able to remove the webAppUrl line entirely and have the server do the right thing by default. The latest config file I mailed you (which works) is very minimalist so that it's more resistant to server config file requirement changes.
Assignee | ||
Comment 34•10 years ago
|
||
Addressed dmose comments.
Carrying forward r+=dmose.
Attachment #8535302 -
Attachment is obsolete: true
Attachment #8535912 -
Flags: review+
Assignee | ||
Comment 35•10 years ago
|
||
I ran the loop tests manually and it should only be cross-platform web content.
In case that is not convincing enough here is a try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=220119c85360
Keywords: checkin-needed
Comment 36•10 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Comment 37•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment 38•10 years ago
|
||
Comment on attachment 8535912 [details] [diff] [review]
bug_1095303_update_to_rooms_ui.patch
Approval Request Comment
[Feature/regressing bug #]: rooms
[User impact if declined]: tests wont' be improved
[Describe test coverage new/current, TBPL]: test-only change, on m-c
[Risks and why]: No risk other than bug in the test.
[String/UUID change made/needed]: none
Attachment #8535912 -
Flags: approval-mozilla-beta?
Attachment #8535912 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
status-firefox37:
--- → fixed
Updated•10 years ago
|
Attachment #8535912 -
Flags: approval-mozilla-beta?
Attachment #8535912 -
Flags: approval-mozilla-beta+
Attachment #8535912 -
Flags: approval-mozilla-aurora?
Attachment #8535912 -
Flags: approval-mozilla-aurora+
Comment 39•10 years ago
|
||
Updated•10 years ago
|
Iteration: --- → 37.1
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•