976116 - Implement Loop client end-to-end call test

Status: RESOLVED FIXED

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

Description

[Adam Roach [:abr]]

Once we know what tools we're using for the Loop client system tests, this bug is a placeholder for implementing the actual tests. It needs sub-bugs for each of the test scenarios we'll be implementing.

Comment 1

[Maire Reavy [:mreavy]]

Dan -- Since you did the investigation for this (bug 976114), can you tell us what tests you recommend?

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

What we did in Talkilla was try to write a functional test to accompany most significant features (and even changes, when it made sense) that we wrote.  At some level, I expect this to be a judgement call for each Bugzilla bug on the part of the coder and the reviewer.

The most obvious ones are the basic stuff, like "end-to-end call works" with the various subtests ("video works", "NAT traversal works".  At more detailed level, bugs that are rooted in bad interactions (or incorrectly defined interfaces) across individual units often want these sorts of tests.

So I guess don't have a set of tests that I specifically recommend, as much as I just suggest we get the Marionette infrastructure up and running as quickly as we reasonably can, and then make the call on a bug by bug basis.

I do expect that there will be two suites, one for tests that need to check that we handle NAT traversal (both functioning and failure), and one for everything else.  The NAT traversal will require SteepleChase.

Comment 3

[Nils Ohlmeier [:drno]]

(In reply to Dan Mosedale (:dmose) from comment #2)
> I do expect that there will be two suites, one for tests that need to check
> that we handle NAT traversal (both functioning and failure), and one for
> everything else.  The NAT traversal will require SteepleChase.

I'm not sure how familiar people are with SteepleChase. So I just want to point out that SteepleChase is just a remote controller for distributing test cases to two machines. We still need to provide the different network environments, because SteepleChase does not provide or even know anything about this.

With that being said the WebRTC Tiger team is currently working on setting up VM's to at least cover basic NAT scenarios. But from a platform perspective the "fix" to make the platform tests pass in these environments is to provide a TURN server as part of the setup.
It probably makes sense to try to re-use the VM's and SteepleChase, but run different tests and use the Loop backend instead of a local TURN server.

Comment 4

[Maire Reavy [:mreavy]]

I'm reassigning to Abr for bug breakdown since this is a dependency of Bug 972025.

Comment 5

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Implement end to end call in one browser window

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8472009 [details] [diff] [review]
Implement end to end call in one browser window

Review of attachment 8472009 [details] [diff] [review]:
-----------------------------------------------------------------

Cancelling review as discussed on irc, due to errors such as:

 0:24.37 TEST_END: MainThread ERROR, expected PASS
Traceback (most recent call last):
  File "/Users/mark/loop/gecko-dev/testing/marionette/client/marionette/marionette_test.py", line 171, in run
    testMethod()
  File "/Users/mark/loop/gecko-dev/browser/components/loop/test/functional/test_1_browser_call.py", line 155, in test_1_browser_call
    feedback_form = self.wait_for_element_displayed(By.CLASS_NAME, "faces")
  File "/Users/mark/loop/gecko-dev/browser/components/loop/test/functional/test_1_browser_call.py", line 44, in wait_for_element_displayed
    .until(lambda m: m.find_element(by, locator).is_displayed())
  File "/Users/mark/loop/gecko-dev/testing/marionette/client/marionette/wait.py", line 143, in until
    cause=last_exc)
TimeoutException: Traceback (most recent call last):
  File "/Users/mark/loop/gecko-dev/testing/marionette/client/marionette/wait.py", line 122, in until
    rv = condition(self.marionette)
  File "/Users/mark/loop/gecko-dev/browser/components/loop/test/functional/test_1_browser_call.py", line 44, in <lambda>
    .until(lambda m: m.find_element(by, locator).is_displayed())
  File "/Users/mark/loop/gecko-dev/testing/marionette/client/marionette/marionette.py", line 1274, in find_element
    response = self._send_message('findElement', 'value', **kwargs)
  File "/Users/mark/loop/gecko-dev/testing/marionette/client/marionette/decorators.py", line 35, in _
    return func(*args, **kwargs)
  File "/Users/mark/loop/gecko-dev/testing/marionette/client/marionette/marionette.py", line 634, in _send_message
    self._handle_error(response)
  File "/Users/mark/loop/gecko-dev/testing/marionette/client/marionette/marionette.py", line 668, in _handle_error
    raise errors.NoSuchElementException(message=message, status=status, stacktrace=stacktrace)
TimeoutException: Timed out after 5.1 seconds, caused by <class 'marionette.errors.NoSuchElementException'>

::: browser/components/loop/content/js/panel.jsx
@@ +235,5 @@
>        // from the react lib.
>        var cx = React.addons.classSet;
> +      var inputCSSClass = cx({
> +        "pending": this.state.pending,
> +         "callUrl": !this.state.pending /* Used in functional testing, signals

Nit: Please put the comment above the line.

::: browser/components/loop/content/shared/js/views.jsx
@@ +227,5 @@
>      getInitialState: function() {
>        return {
>          video: {enabled: false},
> +        audio: {enabled: false},
> +        marionetteHandle: false /* Used by functional test, signals component ready

nit: please put the comment above the line.

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ -185,5 @@
>        } else {
>          var date = (new Date(callUrlInfo.urlCreationDate * 1000));
>          var options = {year: "numeric", month: "long", day: "numeric"};
>          var timestamp = date.toLocaleDateString(navigator.language, options);
> -

nit: Unnecessary whitespace change, please remove from patch

::: browser/components/loop/test/functional/test_1_browser_call.py
@@ +27,5 @@
> +        # Unfortunately, enforcing preferences currently comes with the side
> +        # effect of launching and restarting the browser before running the
> +        # real functional tests.  Bug 1048554 has been filed to track this.
> +        preferences = {
> +            "loop.server": "http://localhost:" + str(LOOP_SERVER_PORT)

As discussed on irc, I think we can set "media.navigator.permission.disabled" to True here, as we don't need to worry about perms for tests.

I also pondered about using fake media but that's not selectable by pref so we can leave it for now.

Comment 7

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Implement end to end call in one browser window

Comment 8

[Andrei Oprea [mention me on [e-mail]]]

Comment on attachment 8473805 [details] [diff] [review]
Implement end to end call in one browser window

Want to make sure race condition is gone

Comment 9

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Implement end to end call in one browser window

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8473862 [details] [diff] [review]
Implement end to end call in one browser window

Review of attachment 8473862 [details] [diff] [review]:
-----------------------------------------------------------------

r=dmose with the review comments addressed.

::: browser/components/loop/build-jsx
@@ +1,1 @@
> +#! /usr/bin/env python2.7

Let's not keep this pinned quite so tightly.

::: browser/components/loop/test/functional/test_1_browser_call.py
@@ +27,5 @@
> +        # effect of launching and restarting the browser before running the
> +        # real functional tests.  Bug 1048554 has been filed to track this.
> +        preferences = {
> +            "loop.server": "http://localhost:" + str(LOOP_SERVER_PORT),
> +            "media.navigator.permission.disabled": "false"

We'll want this to be True (in Pythonic syntax), and I think we'll want a few more prefs too.  Borrowed from Talkilla:

+    # "browser.dom.window.dump.enabled": "true",
+    # "media.peerconnection.default_iceservers": "[]",
+    # "media.peerconnection.use_document_iceservers": "false",
+    "devtools.chrome.enabled": True,
+    "devtools.debugger.prompt-connection": False,
+    "devtools.debugger.remote-enabled": True,
+    # "media.volume_scale": "0"

Note that some of the syntax there is wrong and will need to be corrected to be pythonic and uncommented.

Also, let's hoist the preferences dicts out to a constant.

@@ +42,5 @@
> +             ignored_exceptions=[NoSuchElementException, StaleElementException])\
> +            .until(lambda m: m.find_element(by, locator).is_displayed())
> +        return self.marionette.find_element(by, locator)
> +
> +    # XXX workaround for Marionette bug YYY

Need to actually file a bug here.  :-)

@@ +112,5 @@
> +
> +    def hangup_call(self):
> +        self.marionette.set_context("chrome")
> +        button = self.marionette.find_element(By.CLASS_NAME, "btn-hangup")
> +        sleep(2)

This needs at least an XXX comment linking to it, I think.

@@ +115,5 @@
> +        button = self.marionette.find_element(By.CLASS_NAME, "btn-hangup")
> +        sleep(2)
> +        button.click()
> +
> +    def test_1_browser_call(self):

I think we want to chunk this up into a set of things at the same level of abstraction, probably where each method is something like do_and_verify_foo().

@@ +118,5 @@
> +
> +    def test_1_browser_call(self):
> +        self.switch_to_panel()
> +
> +        # get a call url

This is just a comment to maintain, and I think the code is pretty readable without it. :-)

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Implement end to end call in one browser window, r=dmose

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

https://hg.mozilla.org/integration/fx-team/rev/c1754ed575cb

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c1754ed575cb

Comment 14

[u279076]

Untracking for QA. Please needinfo me to request specific testing.