Closed Bug 1095303 Opened 10 years ago Closed 9 years ago

Loop functional e2e test is broken

Categories

(Hello (Loop) :: General, defect, P1)

defect

Tracking

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

RESOLVED FIXED
mozilla37
Iteration:
37.1
Tracking Status
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)

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?
Blocks: 1095170
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)
Attached patch test_1_browser_call.aurora.patch (obsolete) — Splinter Review
This fixes python errors on the Aurora branch.
Flags: needinfo?(drno)
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.
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.
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.
Blocks: 1029183
backlog: --- → Fx37+
Priority: -- → P1
Whiteboard: [tech-debt]
Blocks: 972025
David, this is the original bug report that our test is broken. Could you attach your patch(es) here?
Flags: needinfo?(dburns)
Attached patch format_fix.patch (obsolete) — Splinter Review
David, I figured that the indentation of switchToFrame() is actually broken. This should put it back to saner levels.
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
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
Attachment #8532294 - Attachment is obsolete: true
Attached patch python_debugger_noautohide.patch (obsolete) — Splinter Review
Just a patch to make the remaining work on this easier...
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)
Attached patch bug_1095303_ice_loopback.patch (obsolete) — Splinter Review
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.
Removing regression window as the marionette problem is fixed, and we now have to update our own test to our latest code.
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)
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
I have a really hacked version of the test locally passing again for me!
This should fix the remaining issue for now.
Note: lot's of short cuts, which should be fixed before landing.
W00t!  Nice work, Nils!
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)
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.
Addressed dmose comments.

Carrying forward r+=dmose.
Attachment #8535302 - Attachment is obsolete: true
Attachment #8535912 - Flags: review+
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
Closed: 9 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?
Attachment #8535912 - Flags: approval-mozilla-beta?
Attachment #8535912 - Flags: approval-mozilla-beta+
Attachment #8535912 - Flags: approval-mozilla-aurora?
Attachment #8535912 - Flags: approval-mozilla-aurora+
Iteration: --- → 37.1
You need to log in before you can comment on or make changes to this bug.