994483 - get loop client unit tests documented & working with Tbpl

Status: VERIFIED FIXED

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

Description

[Dan Mosedale (:dmosedale, :dmose)]

bug 976127 wires up the mocha/chai unit tests for both the desktop-local client and the shared client code to be run by Marionette and errors propagated up in a way that will be displayed meaningfully by the Marionette test runner.

Once that lands on the loop-ui-initial branch (or maybe even master), we'll need to get it wired up and tested to run with Tbpl, which is a bunch of different slices of work that I'll put into the "User Story" comment as a checklist.

Comment 1

[Jonathan Griffin (:jgriffin)]

For testing, you can you cedar, a mozilla-central clone; that's where we green up tests prior to rolling them out everywhere.

Comment 2

[Mark Banner (:standard8)]

Just to note this is currently blocked by bug 976279, so we can't test live yet.

I'd also suggest that aiui the marionette builds are allready running, we can push this to try server before doing work on cedar or wherever.

Comment 3

[Mark Banner (:standard8)]

Actually, the better bug is bug 994485 as we need everything in one place first ;-)

Comment 4

[Maire Reavy [:mreavy]]

Largely coordination with folks on other teams

Comment 5

[Maire Reavy [:mreavy]]

Like bug 994485, this is waiting on (blocked by) 3rd party negotiations to finish and was being driven by Dan.  Dan will likely drive this again (with Standard8) once we're unblocked.

Comment 6

[Mark Banner (:standard8)]

Stealing.

Updating user story - now that we're on Elm, the marionette tests are able to be run locally, but this needs work to make sure we 'ship' the required files and library to the test tarballs and have them available on the builders.

Comment 7

[Mark Banner (:standard8)]

Attachment: WIP Add Loop marionette tests to tbpl

This is the start of a WIP. It hooks up the manifest to the core marionette system, ships the test files, and fixes a random timeout (caused if the tests run slowly).

Currently, this doesn't get tests running in packaged form - I need to find a way of either re-packaging the files under test as "test files" (so that they can be referenced relatively, like we do now), or try changing them to chrome uris.

Comment 8

[Mark Banner (:standard8)]

Attachment: Add Loop marionette tests to tbpl (WIP)

David: Dan told me that you need to sign-off on the main unit-test.ini change, but would you be happy to give feedback and review the rest of the patch as well?

This is a patch that works, though there's a few things I'm not quite sure about:

- The code we are testing is normally run in a non-privilaged context, via about uris, but the files are effectively chrome uris. This means we can't reference them directly from the test files which are also run in a non-privilaged context.

Therefore I've arranged it so that the files under test, as well as being shipped as part of FF, are also shipped as part of the packaged tests. I can't see an issue with this, but maybe someone else can.

- Marionette manifests appear to have no support for 'support-files' that xpcshell uses, therefore I've put the Marionette manifest in a higher level directory than I would normally to allow the files to be packaged as part of tests.

- Marionette's http server appears to have capability to set the directory that it ships from, but not from within tests nor manifest files, so although I could shorten some of the urls passed to the test suite, that doesn't seem possible currently.



Part of the patch also increases the timeout for searching for elements. This is needed because the test pages that run mocha can take a second or two to complete, and hence we need to wait for the complete element to appear (we had to do this with Selenium on Talkilla as well).


There's a try server run of this patch here: https://tbpl.mozilla.org/?tree=Try&rev=c3e129aad2ff

Comment 9

[David Burns :automatedtester]

Comment on attachment 8433621 [details] [diff] [review]
Add Loop marionette tests to tbpl (WIP)

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

::: browser/components/loop/test/shared/frontend_tester.py
@@ +57,5 @@
>          super(BaseTestFrontendUnits, self).setUp()
>  
> +        # The pages run async after load, give ourselves up to 10 seconds
> +        # after page load for the tests to run.
> +        self.marionette.set_search_timeout(10000)

This won't give you a 10 second grace, it will give you up to 10 seconds to find the element when you do marionette.find_element(...)

::: testing/marionette/client/marionette/tests/unit-tests.ini
@@ +27,5 @@
>  [include:../../../../../dom/nfc/tests/marionette/manifest.ini]
>  [include:../../../../../dom/events/test/marionette/manifest.ini]
> +
> +; loop tests
> +[include:../../../../../browser/components/loop/manifest.ini]

I think this is currently only run with b2g emulator tests from mozharness, I think jgriffin might be able to correct me here

Comment 10

[David Burns :automatedtester]

Jonathan,

testing/marionette/client/marionette/tests/unit-tests.ini is only run on Mnw right or am I remembering incorrectly?

Comment 11

[Mark Banner (:standard8)]

(In reply to David Burns :automatedtester from comment #9)
> > +        # The pages run async after load, give ourselves up to 10 seconds
> > +        # after page load for the tests to run.
> > +        self.marionette.set_search_timeout(10000)
> 
> This won't give you a 10 second grace, it will give you up to 10 seconds to
> find the element when you do marionette.find_element(...)

That's fine, I can clarify that comment. 10 seconds should be more than enough at the moment.
> 
> ::: testing/marionette/client/marionette/tests/unit-tests.ini
> @@ +27,5 @@
> >  [include:../../../../../dom/nfc/tests/marionette/manifest.ini]
> >  [include:../../../../../dom/events/test/marionette/manifest.ini]
> > +
> > +; loop tests
> > +[include:../../../../../browser/components/loop/manifest.ini]
> 
> I think this is currently only run with b2g emulator tests from mozharness,
> I think jgriffin might be able to correct me here

The try server runs seemed to be running this, but I might be missing something, of course.

Comment 12

[Jonathan Griffin (:jgriffin)]

(In reply to David Burns :automatedtester from comment #10)
> Jonathan,
> 
> testing/marionette/client/marionette/tests/unit-tests.ini is only run on Mnw
> right or am I remembering incorrectly?

No, those are also run on Mn tests on desktop.

Comment 13

[Mark Banner (:standard8)]

Attachment: Add Loop unit tests to tbpl

Updated the comment to clarify the 10 second timeout.

Comment 14

[David Burns :automatedtester]

Comment on attachment 8434501 [details] [diff] [review]
Add Loop unit tests to tbpl

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

looks good to me

Comment 15

[Mark Banner (:standard8)]

https://hg.mozilla.org/projects/elm/rev/87e1572c63a3

Closing for tracking purposes.

Comment 16

[Mark Banner (:standard8)]

In doing the changes here, I've actually made a mistake and somehow didn't notice that running the tests locally is now broken. Backing out the test_*.py changes fixes running locally, but that would break the tbpl builds.

I'll need to rethink the strategy here, I'll have a look at it later.

Comment 17

[Mark Banner (:standard8)]

Attachment: Fix running marionette tests locally.

I think this will fix the issue running locally. As the packaged builders don't have the build infrastructure we can't do anything with topsrcdir or things like that so we have to work out the location of the srcdir, and then the relative location to where the files are really.

This works locally, and I think it might work remotely - I've pushed it to try server to find out. https://tbpl.mozilla.org/?tree=Try&rev=df2b7e0bb531

In the future, we probably need to see if we can hook properly into the main Marionette server via the manifest files (which isn't possible atm).

Comment 18

[Mark Banner (:standard8)]

Attachment: Fix running marionette tests locally and bug 1021573 Fix intermittent failure due to the server trying to bind to a socket that hasn't been released yet.

Updated patch that incorporates the fix for bug 1021573 - to fix the intermittent failures due to not being able to get the port.

I had tried getting it to re-use the port, but it didn't seem to work, so I went with using a random port which is simple to do with the python server and gets us around the issue. It also means that the same port doesn't have to be available on everyone's machines.

Comment 19

[Mark Banner (:standard8)]

Also, try runs for the tests: https://tbpl.mozilla.org/?tree=Try&rev=e9f19aed5e9f

Comment 20

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8435858 [details] [diff] [review]
Fix running marionette tests locally and bug 1021573 Fix intermittent failure due to the server trying to bind to a socket that hasn't been released yet.

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

::: browser/components/loop/test/desktop-local/test_desktop_all.py
@@ +9,5 @@
>  class TestDesktopUnits(BaseTestFrontendUnits):
>  
>      def setUp(self):
>          super(TestDesktopUnits, self).setUp()
> +        self.set_server_prefix("../desktop-local/")

will this work on Windows?

Comment 21

[Mark Banner (:standard8)]

Attachment: Fix running marionette tests locally and bug 1021573 Fix intermittent failure due to the server trying to bind to a socket that hasn't been released yet v2

Updated to use urllib.pathname2url to translate the path so that it'll work on Windows. Tested on Windows as well.

Comment 22

[Mark Banner (:standard8)]

https://hg.mozilla.org/projects/elm/rev/2fb7df77825f

Comment 23

[Mark Banner (:standard8)]

https://hg.mozilla.org/mozilla-central/rev/87e1572c63a3
https://hg.mozilla.org/mozilla-central/rev/2fb7df77825f

Comment 24

[u279076]

Assuming verified -- needinfo me if you need QA testing.