976114 - implement basic Loop functional test including both client & server pieces

Status: VERIFIED FIXED

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

Description

[Adam Roach [:abr]]

We need to have end-to-end system tests, driven from the client end, in order to ensure proper system behavior. Currently, marionette and steeplechase seem to be the leading contenders, but we need to better understand their properties before we decide to use them.

My understanding is that David Burns and Ted Mielczarek would be the first points of contact for this discussion.

Comment 1

[(not currently active) Ted Mielczarek]

abr and I chatted on IRC, I think Steeplechase will suffice for this for now. The tests are all HTML+JS content, but it does have SpecialPowers enabled, so we can just use that if we need to hit some chrome functions. If that turns out to be unwieldy we can try something different.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

After getting some validation from the owner and maintainer of Marionette, as well as a couple of other automation folks, about the suitability of Marionette for our uses:

* should Just Work in general, even for the few tests where we'll actually want to reach up into XUL chrome and poke XUL buttons to make things happens.  One caveat, however, is that the XUL chrome code isn't as heavily tested, therefore likely less mature, so we may run into a few bugs there.

* there is already a Marionette suite running on Tbpl, and the A-Team folks sounded happy to just include our tests there, and worry about breaking them out into their module later once we get enough tests.

I talked to Ted some more about this.  Steeplechase is effectively a mixin for existing testing frameworks.  So far he's got it running on mozilla-inbound with a few mochitests.  I got the sense that we could just use Marionette alone for the vast majority of our end-to-end tests, and then just mixin Steeplechase for the smaller number of tests that needed to test NAT traversal.  

My guess is that the combination tests would need to be their own suite, since they'd need a specific network setup to actually do that sort of testing.

Ted said it would be some work to make Steeplechase work with Marionette, but he sounded open to doing it.  Clearly, we'll need to get together with him to understand the concrete possibilities there.

Ted, please correct any mistakes or misinterpretations on my end, if you would!

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Note that Marionette is to some degree the just a cleaner implementation of the WebDriver JSON wire protocol driver directly in the browser.  It comes with a Python API, identical to the broadly used WebDriver API, as well as its own JavaScript API.

Talkilla used the Python API to WebDriver, so we're quite familiar with it and the API's various warts.  My understanding is that there are also a large number of tests for desktop Firefox written against this Python API in the tree (and perhaps even already hooked up to Tbpl?).  

David, Stephen, can you correct any misunderstandings I have here?

Comment 4

[(not currently active) Ted Mielczarek]

Your summary of our conversation sounds correct, and I agree with your assessment.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

I'm working on standing up a simple test using Marionette in the https://github.com/adamroach/gecko-dev/tree/marionette-standup branch.  Pull that branch, and then after you build it, execute 

./mach marionette-test browser/components/loop/test/functional/manifest.ini 

from the top-level.  The test runs, but fails, because somehow activating the socialAPI provider in the default browser prefs isn't causing the status button to appear.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Grabbing the bug, since I have some context in my head.  Adam, if that's a mistake, I apologize.  My expectation is that anyone who wishes could grab this bug from me and run with it in our absence.

Comment 7

[Mark Banner (:standard8)]

The missing magic here was to set 'social.enabled' = true.

In testing this there's a one other thing I'm concerned about that I noticed:

* marionette-test only appears to be enabled by default on b2g. However, it does look like tbpl runs against Firefox, so maybe that's not an issue.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Thanks for figuring out the social.enabled thing.  I guess we'll need to do that manually (or at least manually merge the latest gecko-dev to abr's copy).

I filed the enabled-by-default thing as bug 979650 before I left, and it looks like that's almost ready to land.

Comment 9

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to Dan Mosedale (:dmose) from comment #3)
> Note that Marionette is to some degree the just a cleaner implementation of
> the WebDriver JSON wire protocol driver directly in the browser.  It comes
> with a Python API, identical to the broadly used WebDriver API, as well as
> its own JavaScript API.
> 
> Talkilla used the Python API to WebDriver, so we're quite familiar with it
> and the API's various warts.  My understanding is that there are also a
> large number of tests for desktop Firefox written against this Python API in
> the tree (and perhaps even already hooked up to Tbpl?).  
> 
> David, Stephen, can you correct any misunderstandings I have here?

Sorry, but I'm (honestly!) not the best person to help answer this question :-(

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Spent a bunch of today working with and learning about the Marionette python bindings (thanks to dburns for the help!).  It turns out they're not (yet) quite as compatible with the WebDriver bindings as I had hoped.  I'm currently working on getting switchToFrame ported from Talkilla.  Progress, but not there yet.  I have a bunch of notes, and hope to get further tomorrow.

Comment 11

[:shell escalante]

triage existing similar bugs to determine changes needed and overlap

Comment 12

[Maire Reavy [:mreavy]]

Assigning to Abr for bug breakdown since this is part of the Bug 972025 dependency tree.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

I've got a first system test stood up that's not far from functioning. 

https://github.com/dmose/gecko-dev/compare/marionette-systems

Stuff that still needs doing:

* talk to mdas about chrome/content context issues
* auto-start server as part of the test
* general cleanup

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

After discussions with jmaher and dustin, I've come up with a server theory to start with.  We'll see if it survives contact with an attempt to implement it.  As of today, Linux and OS X test machines have node 0.10.21 available.

Current theory: 

in the "build tests" step of the build process
* git clone the server from some known tag, maybe even master
** discuss which one with server folks
* npm install it
* create any necessary config files

in the "mach test" step
* start the server at the beginning of the functional tests
* shut it down at the end
 
in the mach package-test (or test-package?) step
* ensure that all the bits necessary to run the server are included in the test.zip that gets shipped to the test machines

push-to-try and see what happens!

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Some details about what gets installed where: https://github.com/mozilla/build-puppet/blob/master/modules/packages/manifests/nodejs.pp

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

thanks for owning this and making it a reality.  It is nice to see someone tackle a more complex addition of tests.  The details above don't raise any flags from me.

Comment 17

[(not currently active) Ted Mielczarek]

(In reply to Dan Mosedale (:dmose - needinfo? me for responses) from comment #14)
> After discussions with jmaher and dustin, I've come up with a server theory
> to start with.  We'll see if it survives contact with an attempt to
> implement it.  As of today, Linux and OS X test machines have node 0.10.21
> available.
> 
> Current theory: 
> 
> in the "build tests" step of the build process
> * git clone the server from some known tag, maybe even master
> ** discuss which one with server folks
> * npm install it
> * create any necessary config files

Do you mean to do this for every build? On TBPL and locally? Just a few cautions here:
1) we generally dislike pulling resources from elsewhere during the build, as that adds another point of failure (and also makes it hard to do things without a network connection).
2) If you're going to use code from an external repository, you should have the changeset to use  listed in a config file somewhere in mozilla-central. This makes builds reproducible and also bisectable. We do this for Gaia, for example: http://mxr.mozilla.org/mozilla-central/source/b2g/config/gaia.json (The alternative is to store a snapshot of the code in m-c, but that might not be desirable.)

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

:andreio and I spent some time talking to Ted, and ended up with the following modified theory: 

scope down this bug to include "test that runs manually from 'mach marionette-test' given installed but not-running server"
* addressing the various issues mdas surfaced in bug 1025119
* given an environment variable pointing to a working server repo, causing "mach marionette-test" to spin up
the server, run the test, and shutdown the server

file a new bug to:
* write a python script to configure a known tag of the server
** read tag to pull from JSON file in functional test dir
** clone or update an existing server repo
** do "npm install" ("make install?") and any other necessary setup
* incorporate this script into local runs 
* incorporate this script from mozharness (which runs on the test machines)

Some resources:

gaia tests are run by pulling a github repo at a JSON tag.  We can probably find someone in #ateam who knows where to find the code that does that.

node-speedy tests run here; possibly interesting reading:
http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py#948

Of note is that while the Linux and OS test machines have Node servers installed, the Windows machines do not.  Given that effectively all of the loop code is cross-platform, I think we can probably live with not running the functional tests on Windows machines.

Comment 19

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

Attachment: Functionally test getting URL from server

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Andrei and I made lots of progress here today.  We have a functional test starting up a server and passing, given a working dev.json in an existing cloned server tree. Still to do:

* document the requirement for working dev.json in test/README.md
* file spinoff bugs with patches for a couple of server tweaks
* get this test running with a local server, rather than the production one, by using MozProfile to add in a user.js file similar to https://github.com/mozilla/talkilla/blob/master/test/functional/user.js

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

"Get this thing configuring and using a server" has been spun off to bug 1029183, as per comment 18.

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Started changing the standalone server so that it can be easily started as part of this.  What I think is now left to do:

* start up local content server
XX start using new port (done)
** debug shutdown 
** dynamically generate config.js & remove static generation

* review branch changes
* rename test to reflect what it does
* do any necessary refactoring

* update run-all-loop-tests.sh
* figure out sane-default for LOOP_SERVER
* update README

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: stand up basic functional test for a fetching a URL

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 8466345 [details] [diff] [review]
stand up basic functional test for a fetching a URL

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

The r- here is mainly for the config.js changes. I think this will break the server teams configuration set-up, so we need to clarify if this is ok with them or not. Remy/Ben are probably the people to ask.

The rest is reasonable but with comments that need addressing.

It is also unclear from the test code why the browser starts up, and then restarts to run the actual test. A comment somewhere in test_get_url.py to explain that might be useful.

::: browser/components/loop/README.txt
@@ +31,3 @@
>  
> +  export LOOP_SERVER=/Users/larry/src/loop-server
> +  ./mach marionette-test browser/components/loop/functional/manifest.ini

Missing test/ in the middle of this.

::: browser/components/loop/content/js/panel.js
@@ -76,5 @@
>                                __("display_name_dnd_status") :
>                                __("display_name_available_status");
>  
>        return (
> -        React.DOM.div( {className:"footer component-spacer"}, 

Just to note, this file is now bitrotted. Are you also using jsx v0.10?

::: browser/components/loop/standalone/Makefile
@@ +36,5 @@
>  	@echo $(SOURCE_STAMP) > content/VERSION.txt
>  	@echo $(SOURCE_DATE) >> content/VERSION.txt
>  
> +remove_old_config:
> +	@rm -f content/config.js

This means there's no easy option to generate these values as server-side configuration values.

We either need to revert this change, or check with the server team for what they require.

If we do keep the removal, then you need to update the .gitignore file as well.

::: browser/components/loop/test/functional/hanging_threads.py
@@ +1,5 @@
> +import sys
> +import threading
> +import time
> +
> +# XXX Need to address before running under Tbpl

I don't understand what this relates to. This also should specify a bug number for the follow-up.

::: browser/components/loop/test/functional/manifest.ini
@@ +1,1 @@
> +[DEFAULT]

Side note, we might get issues with running this in packaged form with the manifest in this directory, but lets just ignore that until we get there, and we can bear it in mind when we do.

::: browser/components/loop/test/functional/test_get_url.py
@@ +9,5 @@
> +from mozprocess import processhandler
> +import signal
> +import urlparse
> +
> +# XXX black magic to avoid a hang; see hanging_threads.py for details

This should get a bug # for the XXX

@@ +17,5 @@
> +
> +CONTENT_SERVER_PORT = 3001
> +LOOP_SERVER_PORT = 5001
> +
> +# XXX consider what happens when Makefile "runserver" target changes

Its not clear what we need to consider here - if it changes, that should be detected at the time by running tests?

If keeping, this needs a bug # associated with it.

@@ +70,5 @@
> +        os.killpg(os.getpgid(self.content_server.pid), signal.SIGKILL)
> +        os.killpg(os.getpgid(self.loop_server.pid), signal.SIGKILL)
> +
> +class TestGetUrl(MarionetteTestCase):
> +    # XXX Move this to setup class so it doesn't restart the server

Please specify a bug for this.

@@ +95,5 @@
> +        frame = self.marionette.find_element("id", "loop")
> +        self.marionette.switch_to_frame(frame)
> +
> +    # taken from https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L858
> +    # XXX factor out into utility object for use by other tests

I think we should file a bug for this, or attach it to another bug to be done as part of that bug. Please included the bug # on the comment.

@@ +106,5 @@
> +    def test_get_url(self):
> +        self.switch_to_panel()
> +
> +        # get and check for a call url
> +        url_input = self.wait_for_element_displayed("tag name", "input")

I think I'd prefer url_element or url_input_element or something like that.

@@ +112,5 @@
> +        # wait for pending state to finish
> +        self.assertEqual(url_input.get_attribute("class"), "pending",
> +                         "expect the input to be pending")
> +
> +        # get and check the input (the "callUrl" class is only added after

It seems a little sad that we need to add a class in order to detect text being added to the textbox. Is there not a different way we could detect this?

@@ +121,5 @@
> +                            "input is populated with call URL after pending"
> +                            " is finished")
> +
> +        # save the call url
> +        call_url = url_input.get_attribute("value")

This should be moved before the previous check and used there as well.

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8466345 [details] [diff] [review]
stand up basic functional test for a fetching a URL

After discussions with :mostlygeek and :natim on IRC, I think I see how to proceed.  Will needinfo? again if necessary.

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Mark Banner (:standard8) from comment #24)
> It is also unclear from the test code why the browser starts up, and then
> restarts to run the actual test. A comment somewhere in test_get_url.py to
> explain that might be useful.
> 

It comes from the preferences stuff. I've filed bug 1048551 about that behavior, and commented in the code pointing to that.

> ::: browser/components/loop/content/js/panel.js
>
> Just to note, this file is now bitrotted. Are you also using jsx v0.10?

Regenerated against 0.11.1 for landing on fx-team.
 
> ::: browser/components/loop/test/functional/hanging_threads.py
> @@ +1,5 @@
> > +import sys
> > +import threading
> > +import time
> > +
> > +# XXX Need to address before running under Tbpl
> 
> I don't understand what this relates to. This also should specify a bug
> number for the follow-up.

I've added the following comment:

# XXX We want to convince ourselves that the race condition this file works
# around, tracked by bug 1046873, is gone, and get rid of this file entirely
# before we hook this up to Tbpl, so that we don't introduce an intermittent
# failure.

> 
> @@ +17,5 @@
> > +
> > +CONTENT_SERVER_PORT = 3001
> > +LOOP_SERVER_PORT = 5001
> > +
> > +# XXX consider what happens when Makefile "runserver" target changes
> 
> Its not clear what we need to consider here - if it changes, that should be
> detected at the time by running tests?

That comment is obsolete; removing.

> @@ +95,5 @@
> > +        frame = self.marionette.find_element("id", "loop")
> > +        self.marionette.switch_to_frame(frame)
> > +
> > +    # taken from https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L858
> > +    # XXX factor out into utility object for use by other tests
> 
> I think we should file a bug for this, or attach it to another bug to be
> done as part of that bug. Please included the bug # on the comment.

We'll need to do this as part of bug 976116 (end-to-end call), so I've commented the code appropriately and updated the user story there as well.

> @@ +112,5 @@
> > +        # wait for pending state to finish
> > +        self.assertEqual(url_input.get_attribute("class"), "pending",
> > +                         "expect the input to be pending")
> > +
> > +        # get and check the input (the "callUrl" class is only added after
> 
> It seems a little sad that we need to add a class in order to detect text
> being added to the textbox. Is there not a different way we could detect
> this?

In theory, yes.  At the moment, no, due to bug 1048551, which I've added a comment about and linked to.  I'll more data into that bug once this patch is landed and easily linkable.

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: stand up basic functional test for a fetching a URL

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

OK, this latest patch leaves the config target itself mostly alone, and adds a bunch of comments clarify what gets used when so that there's less confusion in the future.

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 8467373 [details] [diff] [review]
stand up basic functional test for a fetching a URL

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

Looks good, r=Standard8

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/81d9152faaec

Comment 32

[u279076]

James, you originally flagged this as needing QA. Is that still the case?

Comment 33

[James Bonacci [:jbonacci]]

:ashughes it looks like the work/testing/verfication has been done, if I am reading all the history correctly. Who will be using and maintaining this going forward?

Comment 34

[u279076]

Thanks James, marking this verified fixed based on comment 33.

Comment 35

[James Bonacci [:jbonacci]]

Who is actually performing the end-to-end testing on a regular basis?
Is it loop-client Dev or QA?

Comment 36

[Dan Mosedale (:dmosedale, :dmose)]

I (and possibly a few others) run it before we check in.  There's a bunch of work to do to get things truly automate; I expect to be filing a bunch of bugs related to that work in the next small number of days.

Comment 37

[James Bonacci [:jbonacci]]

Great update. Thank you.