Loop functional e2e test is broken

RESOLVED FIXED in Firefox 36

Status

Hello (Loop)
General
P1
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: drno, Assigned: drno)

Tracking

unspecified
mozilla37
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox34 wontfix, firefox35 wontfix, firefox36 fixed, firefox37 fixed)

Details

(Whiteboard: [tech-debt])

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

4 years ago
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)

Updated

4 years ago
Blocks: 1095170
(Assignee)

Comment 1

4 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

4 years ago
Created attachment 8519177 [details] [diff] [review]
test_1_browser_call.aurora.patch

This fixes python errors on the Aurora branch.
Flags: needinfo?(drno)
(Assignee)

Comment 4

4 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?
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.
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.
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.
Keywords: regressionwindow-wanted
(Assignee)

Comment 10

4 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.
(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

4 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).
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.
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → affected
(Assignee)

Updated

4 years ago
Blocks: 1029183

Updated

4 years ago
backlog: --- → Fx37+
Priority: -- → P1
Whiteboard: [tech-debt]

Updated

4 years ago
Blocks: 972025
(Assignee)

Comment 14

4 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

4 years ago
Created attachment 8532294 [details] [diff] [review]
format_fix.patch

David, I figured that the indentation of switchToFrame() is actually broken. This should put it back to saner levels.
(Assignee)

Comment 16

4 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
I have found that bug 1107915 is the cause of the frame issue
Depends on: 1107915
Flags: needinfo?(dburns)
I have pushed a patch to try and for review and hopefully can land today
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

4 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

4 years ago
Attachment #8532294 - Attachment is obsolete: true
(Assignee)

Comment 21

4 years ago
Created attachment 8533907 [details] [diff] [review]
python_debugger_noautohide.patch

Just a patch to make the remaining work on this easier...
(Assignee)

Comment 22

4 years ago
Created attachment 8533909 [details] [diff] [review]
bug_1095303_start_a_conversation.patch

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

4 years ago
Created attachment 8533911 [details] [diff] [review]
bug_1095303_ice_loopback.patch

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

4 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
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

4 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

4 years ago
I have a really hacked version of the test locally passing again for me!
(Assignee)

Comment 28

4 years ago
Created attachment 8534813 [details] [diff] [review]
bug_1095303_copy_url_and_join_conversation.patch

This should fix the remaining issue for now.
Note: lot's of short cuts, which should be fixed before landing.
W00t!  Nice work, Nils!
(Assignee)

Comment 30

4 years ago
Created attachment 8535302 [details] [diff] [review]
bug_1095303_update_to_rooms_ui.patch

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

4 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 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+
(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

4 years ago
Created attachment 8535912 [details] [diff] [review]
bug_1095303_update_to_rooms_ui.patch

Addressed dmose comments.

Carrying forward r+=dmose.
Attachment #8535302 - Attachment is obsolete: true
Attachment #8535912 - Flags: review+
(Assignee)

Comment 35

4 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
https://hg.mozilla.org/mozilla-central/rev/6953b6a787ce
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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?
status-firefox34: affected → wontfix
status-firefox37: --- → fixed
Attachment #8535912 - Flags: approval-mozilla-beta?
Attachment #8535912 - Flags: approval-mozilla-beta+
Attachment #8535912 - Flags: approval-mozilla-aurora?
Attachment #8535912 - Flags: approval-mozilla-aurora+

Updated

3 years ago
Iteration: --- → 37.1
status-firefox35: affected → wontfix
status-firefox36: affected → fixed
You need to log in before you can comment on or make changes to this bug.