Closed Bug 1301781 Opened 6 years ago Closed 6 years ago

Migrate Ping Server to Marionette

Categories

(Toolkit :: Telemetry, defect, P3)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: Silne30, Assigned: Silne30)

References

(Blocks 1 open bug)

Details

(Whiteboard: [measurement:client])

Attachments

(2 files, 6 obsolete files)

We have a current implementation of the ping server for testing that was written for a custom test harness. We are looking to migrate that ping server to work with the marionette harness. 

Original Ping Server:
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/session/runner/ping_server.py

Destination:
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/httpd.py

There will need to be some work done to make sure that this ping server conforms to what is implemented in Marionette using the wptserve package.
Assignee: nobody → jdorlus
Whiteboard: [measurement:client]
Priority: -- → P1
(In reply to John Dorlus [:Silne30] from comment #0)
> Destination:
> https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/
> marionette/runner/httpd.py
> 
> There will need to be some work done to make sure that this ping server
> conforms to what is implemented in Marionette using the wptserve package.

When I mentioned wptserve I didn't say that you have to rewrite the server! What you need are actually your own setup request handlers, so that you can listen for any ping request.
(In reply to Henrik Skupin (:whimboo) from comment #1)
> (In reply to John Dorlus [:Silne30] from comment #0)
> > Destination:
> > https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/
> > marionette/runner/httpd.py
> > 
> > There will need to be some work done to make sure that this ping server
> > conforms to what is implemented in Marionette using the wptserve package.
> 
> When I mentioned wptserve I didn't say that you have to rewrite the server!
> What you need are actually your own setup request handlers, so that you can
> listen for any ping request.

Thanks for the clarification.
Status: NEW → ASSIGNED
Attached patch patch.diff (obsolete) — Splinter Review
Can you take a look at this? Specifically, run_test.py TelemetryTestHarness for the handler.
Attachment #8798680 - Flags: review?(hskupin)
Depends on: 1308375
Comment on attachment 8798680 [details] [diff] [review]
patch.diff

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

In general I love to see mozreview requests which helps a lot. But its fine this time for getting the new code implemented.

I did a quick skim over the code and I think it's more a feedback request than an actual review request at this stage.

::: testing/marionette/harness/marionette/pings/sample_ping
@@ +1,1 @@
> +{"a" : "b"}
\ No newline at end of file

Why is that needed under marionette?

::: toolkit/components/telemetry/tests/requirements.txt
@@ +1,3 @@
> +blessings==1.6
> +browsermob-proxy==0.7.1
> +firefox-puppeteer==52.0.0

Please do not a hard-coding here if you really really need puppeteer. From my perspective I think that you shouldn't use it given that telemetry tests aren't actually testing lots of ui, right?

@@ +16,5 @@
> +moztest==0.7
> +mozversion==1.4
> +requests==2.11.1
> +simplejson==3.8.2
> +wptserve==1.4.0

You should not list all the dependencies from marionette again unless you need one of those components in your own modules.

::: toolkit/components/telemetry/tests/setup.py
@@ +28,5 @@
> +          'Topic :: Software Development :: Libraries :: Python Modules',
> +      ],
> +      keywords='mozilla',
> +      author='Firefox Test Engineering Team',
> +      author_email='tools@lists.mozilla.org',

Those two lines do not apply. We are not willed to own those tests and the testcase classes.

::: toolkit/components/telemetry/tests/telemetry_harness/runtests.py
@@ +25,5 @@
> +    def __init__(self, **kwargs):
> +        BaseMarionetteTestRunner.__init__(self, **kwargs)
> +        if not self.server_root:
> +            self.server_root = doc_root
> +        self._server = self.create_httpd(False)

I do not see why you have to create another instance of wptserve. You should be able to extend the existing server's instance for your new handler.

@@ +34,5 @@
> +        self.app = 'fxdesktop'
> +        self.test_handlers = [TelemetryTestCase]
> +
> +    @staticmethod
> +    def unpack(incoming):

This code is not related to the runner. You should split it out into its own module.

@@ +50,5 @@
> +
> +    @staticmethod
> +    def strip_url_for_filename(url):
> +        file_name = url[46:].replace('/', '.')
> +        return file_name

Same here.

@@ +69,5 @@
> +
> +
> +class TelemetryTestArguments(BaseMarionetteArguments):
> +    def __init__(self, **kwargs):
> +        BaseMarionetteArguments.__init__(self, **kwargs)

If there is no extra code added you can leave out the override of those methods. I think what you really only need is the customized TelemetryTestRunner class.

::: toolkit/components/telemetry/tests/telemetry_harness/testcase.py
@@ +21,5 @@
> +        self.puppeteer = None
> +        self.browser = None
> +
> +    def setUp(self, *args, **kwargs):
> +        MarionetteTestCase.setUp(self)

Always use `super()`.

@@ +29,5 @@
> +        # enable browser toolbox
> +        self.puppeteer.prefs.set_pref('devtools.chrome.enabled', True)
> +        self.puppeteer.prefs.set_pref('devtools.debugger.remote-enabled', True)
> +        # prevent ui popups auto-hiding
> +        self.puppeteer.prefs.set_pref('ui.popup.disable_autohide', True)

Please do not use puppeteer to set preferences but marioentte directly. Also make sure to reset those at the end.

If all of those are hard-requirements, you should better move it out to the runner. See our update tests runner as example:
https://dxr.mozilla.org/mozilla-central/source/testing/firefox-ui/harness/firefox_ui_harness/runners/update.py

@@ +36,5 @@
> +
> +        self.marionette.set_pref('datareporting.policy.dataSubmissionPolicyBypassNotification', True)
> +
> +    def tearDown(self, *args, **kwargs):
> +        self.marionette.close()

Why is that needed?

Keep in mind that you also have to call tearDown of the super class.
Attachment #8798680 - Flags: review?(hskupin) → feedback-
Attached patch wip_patch.diff (obsolete) — Splinter Review
Still a work in progress. But you can take a look at what I have so far.
Attachment #8798680 - Attachment is obsolete: true
Attachment #8799037 - Flags: review?(hskupin)
I uploaded the patch so you could let me know if you understand why the self.marionette_weakref() call is saying throwing an unresolved reference error. I made sure to call super() but perhaps you can spot where I went wrong.
Comment on attachment 8799037 [details] [diff] [review]
wip_patch.diff

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

This new version of the patch looks definitely better! Good to see that you got rid of this extra server instance which drops the complexity a lot. I still see a couple of things to work on, and especially the part of making the default httpd server available for the Marionette class. For more details please see my inline comments.

Given that this is still a WIP please refrain from asking for review but ask for feedback. Thanks.

::: testing/marionette/harness/marionette/runner/base.py
@@ +1039,5 @@
>          testloader = unittest.TestLoader()
>          suite = unittest.TestSuite()
>          self.test_kwargs['expected'] = expected
>          self.test_kwargs['test_container'] = test_container
> +        self.test_kwargs['httpd'] = self.httpd

Ok, so I just had a look at the current implementation of the server inside the runner. And I noticed that only offering the `baseurl` in the Marionette object is reducing our capabilities a lot. It means no test could setup its own handlers or tweak the server for specific behavior eg. slow responses.

I would suggest that you file a new bug for Marionette which will implement such a change. Please don't do it here in this patch. It might mean a bit of refactoring, where we would have to get feedback from Andreas and David first.

Attaching to the test as done above, I don't think is the right way.

::: toolkit/components/telemetry/tests/telemetry_harness/testcase.py
@@ +26,5 @@
>  
>      def setUp(self, *args, **kwargs):
> +        super(TelemetryTestCase, self).setUp()
> +        self.logger.info(self.httpd)
> +        self.httpd.routes.extend([("POST", re.compile('/pings.*'), pings)])

Keep in mind that you want to remove this extra route in tearDown. Otherwise you add more and more handlers if there is no internal check for duplicates.

@@ +28,5 @@
> +        super(TelemetryTestCase, self).setUp()
> +        self.logger.info(self.httpd)
> +        self.httpd.routes.extend([("POST", re.compile('/pings.*'), pings)])
> +
> +        self.prefs['toolkit.telemetry.server'] = 'http://127.0.0.1:{}/pings'.format(self.httpd.port)

The server also allows you to retrieve the hostname, so you shouldn't hard-code it here.

@@ +58,5 @@
> +        return plain_data
> +
> +    @staticmethod
> +    def strip_url_for_filename(url):
> +        file_name = url[46:].replace('/', '.')

What is that exactly doing? It looks kinda error-prone to me with the hardcoded index.

@@ +66,5 @@
> +@handlers.handler
> +def pings(request, response):
> +    file_name = TelemetryTestCase.strip_url_for_filename(request.url) + '.json'
> +    json_data = json.loads(TelemetryTestCase.unpack(request))
> +    with open(doc_root + '/' + file_name, 'w+') as json_file:

Think about that this will pollute your doc_root. I don't see that you clean that up between tests. I better would suggest to use a temporary folder to save those pings to.
Attachment #8799037 - Flags: review?(hskupin) → feedback+
Depends on: 1309318
Blocks: 1301776
Attached patch wip2_patch.diff (obsolete) — Splinter Review
This patch is an updated version of the previous. It makes the changes you were suggesting. You will see that there is an httpd server that we spin up manually. That is only a workaround until the blocker bug is resolved.
Attachment #8799037 - Attachment is obsolete: true
Attachment #8806374 - Flags: review?(hskupin)
Comment on attachment 8806374 [details] [diff] [review]
wip2_patch.diff

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

With the dep fixed soon, this looks to be on the right track. Comments see inline.

John, please refrain from asking for review for patches which are not done yet. In those cases it's really a feedback request. Thanks.

::: testing/mozbase/mozinstall/mozinstall/mozinstall.py
@@ -138,1 @@
>          # any other kind of exception like KeyboardInterrupt is just re-raised.

Please undo all the changes to this file and others for mozinstall. Those are really not related to your additions.

::: toolkit/components/telemetry/tests/requirements.txt
@@ +1,1 @@
> +blessings==1.6

I think we should use an additional subfolder called `marionette` here.

::: toolkit/components/telemetry/tests/telemetry_harness/runtests.py
@@ +17,5 @@
> +        # if not self.server_root:
> +        #     self.server_root = doc_root
> +        # pick up prefs from marionette_driver.geckoinstance.DesktopInstance
> +        self.app = 'fxdesktop'
> +        self.test_handlers = [TelemetryTestCase]

If you only create the harness abstraction to allow the TelemetryTestCase I would suggest you get rid of it and place a new module next to the tests which contains its definition. That way your tests can rely on pure Marionette without any additional layer in between which only causes maintenance costs.

::: toolkit/components/telemetry/tests/telemetry_harness/testcase.py
@@ +7,5 @@
> +import simplejson as json
> +import time
> +
> +from firefox_puppeteer import Puppeteer
> +from firefox_puppeteer.testcases import BaseFirefoxTestCase

Just a note that this will change soon. See bug 1313312.

@@ +18,5 @@
> +from marionette_driver.by import By
> +
> +
> +from wptserve import handlers
> +from wptserve.server import WebTestHttpd

Given that this code won't be necessary anymore soonish, I won't comment what might have to change here.

@@ +20,5 @@
> +
> +from wptserve import handlers
> +from wptserve.server import WebTestHttpd
> +
> +import helpers

You are importing a global package. If you want the helper.py from the same folder you will have to change that. Otherwise another package with the name "helpers" can conflict here.

@@ +48,5 @@
> +
> +        # Set up puppeteer
> +        self.puppeteer = Puppeteer()
> +        self.puppeteer.marionette = self.marionette
> +        self.browser = self.puppeteer.windows.current

Those lines should be moved below the forced restart.

@@ +70,5 @@
> +        self.marionette.enforce_gecko_prefs(telemetry_prefs)
> +
> +    def tearDown(self, *args, **kwargs):
> +        super(TelemetryTestCase, self).tearDown()
> +        self.marionette.close()

If a test needs to clean-up left-open tabs it has to be done in the test, or as fallback here but then you have to check the amount of open tabs.

@@ +82,5 @@
> +@handlers.handler
> +def pings(request, response):
> +    json_data = json.loads(helpers.unpack(request))
> +    TelemetryTestCase.pings.append(json_data)
> +    return 200

You should make this method part of the testcase. You do not have to necessarily add the decorator because you already add it manually above.
Attachment #8806374 - Flags: review?(hskupin) → feedback+
Comment on attachment 8806374 [details] [diff] [review]
wip2_patch.diff

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

Commenting here on what we discussed yesterday.

::: toolkit/components/telemetry/tests/telemetry_marionette/test_telemetry_ping.py
@@ +19,5 @@
> +            self.browser.tabbar.open_tab()
> +            assert len(self.browser.tabbar.tabs) == 2
> +        self.trigger_ping()
> +        self.logger.info(TelemetryTestCase.pings)
> +        assert len(TelemetryTestCase.pings) > 0

To have this work reliably, we need to be able to wait for the next incoming ping.
This could e.g. work similarly to what we do in xpcshell-tests:
https://dxr.mozilla.org/mozilla-central/rev/ade8d4a63e57560410de106450f37b50ed71cca5/toolkit/components/telemetry/tests/unit/head.js#93

This helper allows tests to just do:
let pingData = yield PingServer.promiseNextPing();

Once the ping is received, this should assert the ping type (ping.type == "your-testping-type").
I think I covered most of everything you had asked in the previous feedback session. Please let me know if this looks better.
Attachment #8806374 - Attachment is obsolete: true
Attachment #8821324 - Flags: feedback?(hskupin)
Python is not necessarily my strongest language. I did my best to emulate what was in head.js. Let me know if this works or not.
Flags: needinfo?(gfritzsche)
Comment on attachment 8821324 [details] [diff] [review]
Added_asynchronous_wait_for_test_.patch

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

This looks like a good start.

::: toolkit/components/telemetry/tests/marionette_tests/telemetry_harness/testcase.py
@@ +55,5 @@
> +            'toolkit.telemetry.log.level':  0,
> +            'toolkit.telemetry.log.dump': True
> +        }
> +
> +        self.marionette.enforce_gecko_prefs(telemetry_prefs)

Will these be enforced before Firefox launches?

@@ +71,5 @@
> +        time_for_ping = 0
> +        p = Process(target=self._send_ping)
> +        p.start()
> +        p.join()
> +        while not self.received_ping(current_num_pings):

Do we have any better async pogramming patterns in Python 2 & Marionette?
Henrik might know?

@@ +80,5 @@
> +            time_for_ping += 1
> +
> +    def _send_ping(self):
> +        self.marionette.execute_script('Cu.import("resource://gre/modules/TelemetryController.jsm", this);'
> +                                       'TelemetryController.submitExternalPing("heartbeat", {});')

I think individual test-cases should control how they send pings & which.

::: toolkit/components/telemetry/tests/marionette_tests/telemetry_tests/test_telemetry_ping.py
@@ +17,5 @@
> +        with self.marionette.using_context(self.marionette.CONTEXT_CHROME):
> +            self.browser.tabbar.open_tab()
> +            assert len(self.browser.tabbar.tabs) == 2
> +        self.trigger_ping()
> +        assert len(TelemetryTestCase.ping_list) > 0

What we want for test cases is a pattern like:
# (1) ... test case somehow triggers ping
# (2) wait for ping being received, e.g. something like:
ping = self.wait_for_ping()
# (3) ... test checks ping contents etc.

(1) should be up to the test. It could e.g. happen through "flip a pref" or "install an addon".
(2) is what TelemetryTestCase should implement.

::: toolkit/components/telemetry/tests/marionette_tests/telemetry_harness/telemetry_helpers.py
@@ +6,5 @@
> +import gzip
> +import os
> +
> +
> +def unpack(incoming):

This doesn't seem to be used outside of testcase.py, maybe it should just live there?
Flags: needinfo?(gfritzsche)
Attached patch Updates_Per_georg.patch (obsolete) — Splinter Review
Made small updates per Georg. I haven't implemented the addons install portion yet but most of the other changes have been made.
Attachment #8821324 - Attachment is obsolete: true
Attachment #8821324 - Flags: feedback?(hskupin)
Attachment #8821560 - Flags: feedback?(gfritzsche)
Attachment #8821560 - Flags: feedback?(gfritzsche) → feedback?(alessio.placitelli)
Comment on attachment 8821560 [details] [diff] [review]
Updates_Per_georg.patch

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

This looks good as a base. Please address the feedback from Georg.

::: toolkit/components/telemetry/tests/marionette_tests/telemetry_tests/__init__.py
@@ +1,1 @@
> +
\ No newline at end of file

\ No newline at end of file

This is missing the licensing note (the one you used in the other __init__.py file).

::: toolkit/components/telemetry/tests/marionette_tests/telemetry_harness/testcase.py
@@ +56,5 @@
> +            'toolkit.telemetry.log.level':  0,
> +            'toolkit.telemetry.log.dump': True
> +        }
> +
> +        self.marionette.enforce_gecko_prefs(telemetry_prefs)

As Georg mentioned, will these be enforced before Firefox launches? 

It would be useful to add a comment above this line for future references and doubts as well.

@@ +70,5 @@
> +    def trigger_ping(self, method, ping_type):
> +        current_num_pings = len(TelemetryTestCase.ping_list)
> +        time_for_ping = 0
> +        try:
> +            p = Process(target=self._send_ping, args=[method, ping_type])

As mentioned by Georg, this and |_send_ping| should probably be handled by each individual test case.

Instead of directly calling the Telemetry API, test cases should test the behaviour that triggers sending the ping (e.g. changing a watched preferences or installing an addon).

@@ +73,5 @@
> +        try:
> +            p = Process(target=self._send_ping, args=[method, ping_type])
> +            p.start()
> +            p.join()
> +            while not self.received_ping(current_num_pings):

Quoting Georg: "Do we have any better async pogramming patterns in Python 2 & Marionette?
Henrik might know?"
Attachment #8821560 - Flags: feedback?(alessio.placitelli)
(In reply to Alessio Placitelli [:Dexter] from comment #15)
> Comment on attachment 8821560 [details] [diff] [review]
> Updates_Per_georg.patch
> 
> Review of attachment 8821560 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good as a base. Please address the feedback from Georg.
> 
> :::
> toolkit/components/telemetry/tests/marionette_tests/telemetry_tests/__init__.
> py
> @@ +1,1 @@
> > +
> \ No newline at end of file
> 
> \ No newline at end of file
> 
> This is missing the licensing note (the one you used in the other
> __init__.py file).

The __init__.py files have been addressed along with the License.

> :::
> toolkit/components/telemetry/tests/marionette_tests/telemetry_harness/
> testcase.py
> @@ +56,5 @@
> > +            'toolkit.telemetry.log.level':  0,
> > +            'toolkit.telemetry.log.dump': True
> > +        }
> > +
> > +        self.marionette.enforce_gecko_prefs(telemetry_prefs)
> 
> As Georg mentioned, will these be enforced before Firefox launches? 
> 
> It would be useful to add a comment above this line for future references
> and doubts as well.


Added a comment to address the fact that enforcing the telemetry prefs forces a restart of Firefox.


> @@ +70,5 @@
> > +    def trigger_ping(self, method, ping_type):
> > +        current_num_pings = len(TelemetryTestCase.ping_list)
> > +        time_for_ping = 0
> > +        try:
> > +            p = Process(target=self._send_ping, args=[method, ping_type])


I added a conditional branch that will allow for the test case to select the method of sending a ping.

ping = self.trigger_ping(method, ping_type)

I am working on the implementation of the addon being installed and by flipping a pref. The JavaScript API will remain as an option but only as a last resort (handled with a conditional statement)


> 
> Instead of directly calling the Telemetry API, test cases should test the
> behaviour that triggers sending the ping (e.g. changing a watched
> preferences or installing an addon).
> 
> @@ +73,5 @@
> > +        try:
> > +            p = Process(target=self._send_ping, args=[method, ping_type])
> > +            p.start()
> > +            p.join()
> > +            while not self.received_ping(current_num_pings):
> 
> Quoting Georg: "Do we have any better async pogramming patterns in Python 2
> & Marionette?
> Henrik might know?"


As far as this async pattern goes, I have definitely reached out to different developers for different ways to do this. So far, this is all I came up with. I would definitely welcome help. I will NI Henrik for response once he gets back.
Flags: needinfo?(hskupin)
Once I get clarification on that, I will repost for feedback.
Comment on attachment 8821560 [details] [diff] [review]
Updates_Per_georg.patch

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

::: toolkit/components/telemetry/tests/marionette_tests/requirements.txt
@@ +16,5 @@
> +moztest==0.7
> +mozversion==1.4
> +requests==2.11.1
> +simplejson==3.8.2
> +wptserve==1.4.0

There is no need to list all of those dependencies unless you make use of them directly in the telemetry harness. So please remove all which do not have to explicitly be listed here.

::: toolkit/components/telemetry/tests/marionette_tests/conftest.py
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +import pytest

So you flipped to using pytest in tree? Please be aware that we do not support this yet for the Marionette harness. In case something is broken or cannot be accessed it's on your side to get support added.

It would have been great to have sticked with marionette_harness given that this adds another dozen of new lines to the patch, which actually completely changes it. Reviewing all the changes is a pain, so please really setup your mozreview config and push it there for review. Thanks!

@@ +12,5 @@
> +def pytest_addoption(parser):
> +    parser.addoption(
> +        '--bin',
> +        default='/Applications/FirefoxNightly.app/Contents/MacOS/firefox-bin',
> +        help='path for Firefox binary')

I don't see why this option is helpful. It only defines a custom Firefox binary for a single platform. You should get rid of it.

@@ +16,5 @@
> +        help='path for Firefox binary')
> +    parser.addoption(
> +        '--base-url',
> +        metavar='url',
> +        help='base url for the application under test.'

What is that used for?

@@ +23,5 @@
> +
> +@pytest.fixture
> +def marionette(request, timeout):
> +    """Return a marionette instance"""
> +    m = Marionette(bin=request.config.option.bin)

This overrides everything we do in the Marionette harness. So you don't want to rely on it at all?

@@ +34,5 @@
> +
> +@pytest.fixture
> +def timeout():
> +    """Return default timeout"""
> +    return TIMEOUT

Why does it return a constant and not an actual value?

::: toolkit/components/telemetry/tests/marionette_tests/__init__.py
@@ +1,1 @@
> +
\ No newline at end of file

License is missing.

::: toolkit/components/telemetry/tests/marionette_tests/telemetry_tests/test_telemetry_ping.py
@@ +9,5 @@
> +    def setUp(self):
> +        super(TestPingServer, self).setUp()
> +
> +    def tearDown(self):
> +        super(TestPingServer, self).tearDown()

Nothing is contained in those two methods, so they are not needed at all.

::: toolkit/components/telemetry/tests/marionette_tests/telemetry_harness/testcase.py
@@ +5,5 @@
> +import gzip
> +import os
> +import re
> +import time
> +from multiprocessing import Process

Never use multiprocessing directly. Instead import it from __future__ if you really need it.

@@ +18,5 @@
> +here = os.path.abspath(os.path.dirname(__file__))
> +doc_root = os.path.join(os.path.dirname(here), "www")
> +
> +
> +class TelemetryTestCase(MarionetteTestCase, PuppeteerMixin):

The mixin class has to be listed first.

@@ +29,5 @@
> +        super(TelemetryTestCase, self).__init__(*args, **kwargs)
> +        self.puppeteer = None
> +        self.browser = None
> +        self.telemetry_server_address = None
> +        self.httpd = httpd.FixtureServer(doc_root)

I would kill this whole __init__ method given that you initialize those variables in setUp anyway.

@@ +36,5 @@
> +        super(TelemetryTestCase, self).setUp()
> +
> +        # Set up puppeteer
> +        self.puppeteer = Puppeteer(self.marionette)
> +        self.browser = self.puppeteer.windows.current

This line is plainly wrong. It should get the browser from self.puppeteer.browser. What you have here could be any kind of window.

@@ +41,5 @@
> +
> +        # Start and configure server
> +        self.httpd.start()
> +        ping_route = [("POST", re.compile('/pings.*'), self.pings)]
> +        self.httpd.routes.extend(ping_route)

It's great to see that this works that easily now with an own httpd fixture server.

@@ +43,5 @@
> +        self.httpd.start()
> +        ping_route = [("POST", re.compile('/pings.*'), self.pings)]
> +        self.httpd.routes.extend(ping_route)
> +
> +        self.telemetry_server_address = '{}pings'.format(self.httpd.get_url())

The variable name is not clear to me. Maybe you want to use `self.ping_url` here.

@@ +48,5 @@
> +
> +        telemetry_prefs = {
> +            'toolkit.telemetry.server': self.telemetry_server_address,
> +            'toolkit.telemetry.initDelay': '1',
> +            "toolkit.telemetry.enabled": True,

Telemetry is still enabled by default in Marionette tests. We only point the server url to a custom one. So this line is not needed.

@@ +51,5 @@
> +            'toolkit.telemetry.initDelay': '1',
> +            "toolkit.telemetry.enabled": True,
> +            'datareporting.healthreport.uploadEnabled': True,
> +            'datareporting.policy.dataSubmissionEnabled': True,
> +            'datareporting.policy.dataSubmissionPolicyBypassNotification': True,

Please check those others too in geckoinstance.py.

@@ +56,5 @@
> +            'toolkit.telemetry.log.level':  0,
> +            'toolkit.telemetry.log.dump': True
> +        }
> +
> +        self.marionette.enforce_gecko_prefs(telemetry_prefs)

It will set the prefs and do a restart of Firefox.

@@ +59,5 @@
> +
> +        self.marionette.enforce_gecko_prefs(telemetry_prefs)
> +
> +    def received_ping(self, num_pings):
> +        return len(self.ping_list) > num_pings

I don't see the usefulness of this method. I would suggest that tests access self.pings directly.

@@ +70,5 @@
> +    def trigger_ping(self, method, ping_type):
> +        current_num_pings = len(TelemetryTestCase.ping_list)
> +        time_for_ping = 0
> +        try:
> +            p = Process(target=self._send_ping, args=[method, ping_type])

Well, I do not understand why this code is needed at all. Why do we need a separate process here?

@@ +89,5 @@
> +        if method == 'install':
> +            pass
> +        else:
> +            self.marionette.execute_script('Cu.import("resource://gre/modules/TelemetryController.jsm", this);'
> +                                           'TelemetryController.submitExternalPing("' + ping_type + '", {});')

Use a doctype like string to have it formatted better. There is also a script_args argument for execute_script, which has to be used to pass in parameters.

@@ +101,5 @@
> +    def unpack(incoming):
> +        post_data = incoming.body
> +        if "Content-Encoding" in incoming.headers and incoming.headers["Content-Encoding"] == "gzip":
> +            gz_data = post_data
> +            with open('temp.gz', 'wb') as gz_file:

If you need temporary files those should be created in the temp folder. Otherwise I would suggest to use a stream here.
Attachment #8821560 - Flags: feedback-
Flags: needinfo?(hskupin)
This bug looks related to bug 1284268.
Decompressing gzipped data can be done in memory:
  plainData = zlib.decompress(postData, zlib.MAX_WBITS | 16)

The last parameter may be optional.
Andre(In reply to André Reinald from comment #20)
> Decompressing gzipped data can be done in memory:
>   plainData = zlib.decompress(postData, zlib.MAX_WBITS | 16)
> 
> The last parameter may be optional.

I really appreciate the help.
Priority: P1 → P3
Attachment #8848582 - Flags: review?(hskupin)
Flags: needinfo?(gfritzsche)
Preferences enabled during test: 
telemetry_prefs = {
            'toolkit.telemetry.enabled': True,
            'toolkit.telemetry.send.overrideOfficialCheck': True,
            'toolkit.telemetry.server': self.ping_url,
            'toolkit.telemetry.initDelay': '1',
            'datareporting.healthreport.uploadEnabled': True,
            'datareporting.policy.dataSubmissionEnabled': False,
            'datareporting.policy.dataSubmissionPolicyBypassNotification': True,
            'toolkit.telemetry.log.level': 0,
            'toolkit.telemetry.log.dump': True
        }
I made sure to set MOZ_TELEMETRY_REPORTING=1 in a mozconfig file before building. 

I have also attached the gecko logs from marionette.
Attachment #8821560 - Attachment is obsolete: true
Attachment #8850619 - Flags: feedback?(gfritzsche)
Found the issue. For some reason, data submission was disabled. Got that error fixed and pings are working now.
Flags: needinfo?(gfritzsche)
I assume this needs an update of the patch. Just asking before I start a review in the middle of a next update.
Yes. I that will need to be updated.
Comment on attachment 8848582 [details]
Bug 1301781 - Add test harness for telemetry tests

https://reviewboard.mozilla.org/r/121492/#review126474

The current implementation looks way cleaner now. So it shouldn't take that long anymore until we can call this done. In the following you can find a couple of review comments.

::: commit-message-34585:1
(Diff revision 3)
> +Bug 1301781 - Submitted patch for converting ping server to marionette witha  small test to ensure that the server works.

Please shorten the decription to something like: `Add test harness for telemetry tests`.

Then add a more details description after an empty line what's actually contained in this patch, and which explains all that good enough.

::: toolkit/components/telemetry/tests/marionette/harness/__init__.py:3
(Diff revision 3)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.

I would still propose that you add a subfolder here like `tests/marionette/harness/telemetry_harness`. It's a better structure for your Python package.

Only `setup.py` and `requirements.txt` should remain at the current location. Btw. the latter file I cannot find in this patch.

::: toolkit/components/telemetry/tests/marionette/harness/setup.py:18
(Diff revision 3)
> +
> +def read(*parts):
> +    with open(os.path.join(THIS_DIR, *parts)) as f:
> +        return f.read()
> +
> +setup(name='telemetry-tests',

This package is the harness and not the tests.

::: toolkit/components/telemetry/tests/marionette/harness/testcase.py:18
(Diff revision 3)
> +from marionette_harness.runner import httpd
> +
> +
> +here = os.path.abspath(os.path.dirname(__file__))
> +doc_root = os.path.join(os.path.dirname(here), "www")
> +temp_dir = os.path.join(os.path.dirname(here), "temp")

For temporary directories do not use a folder inside the tree. Instead there is the `tempfile` package available.

::: toolkit/components/telemetry/tests/marionette/harness/testcase.py:34
(Diff revision 3)
> +        self.httpd = httpd.FixtureServer(doc_root)
> +        self.httpd.start()
> +        ping_route = [("POST", re.compile('/pings.*'), self.pings)]
> +        self.httpd.routes.extend(ping_route)
> +
> +        self.ping_url = '{}pings'.format(self.httpd.get_url('/'))

From reading the code I assume this is the base URL, and tests will have to add a suffix? If that is the case please rename the property appropriately.

::: toolkit/components/telemetry/tests/marionette/harness/testcase.py:48
(Diff revision 3)
> +            'toolkit.telemetry.log.level': 0,
> +            'toolkit.telemetry.log.dump': True
> +        }
> +
> +        # Firefox will be forced to restart with the prefs enforced.
> +        self.marionette.enforce_gecko_prefs(telemetry_prefs)

Given that those prefs (beside toolkit.telemetry.server) will apply to all telemetry tests you should better move everything to the harness itself and add your own default prefs there. This wouldn't require an extra restart of Firefox and saves lots of time.

See `firefox_ui_harness/runners/update.py`.

::: toolkit/components/telemetry/tests/marionette/harness/testcase.py:50
(Diff revision 3)
> +        }
> +
> +        # Firefox will be forced to restart with the prefs enforced.
> +        self.marionette.enforce_gecko_prefs(telemetry_prefs)
> +        if not os.path.exists(temp_dir):
> +            os.makedirs(temp_dir)

See above. Make use of `tempfile` instead.

::: toolkit/components/telemetry/tests/marionette/harness/testcase.py:53
(Diff revision 3)
> +        self.marionette.enforce_gecko_prefs(telemetry_prefs)
> +        if not os.path.exists(temp_dir):
> +            os.makedirs(temp_dir)
> +
> +    def tearDown(self, *args, **kwargs):
> +        super(TelemetryTestCase, self).tearDown()

First run the clean-up steps of your own testcase class, then call `super()`.

::: toolkit/components/telemetry/tests/marionette/harness/testcase.py:65
(Diff revision 3)
> +        self.ping_list.append(json_data)
> +        return 200
> +
> +
> +def unpack(incoming):
> +    post_data = incoming.body

You should pass in `request.body` directly, so that the method can have a parameter like `data`.

::: toolkit/components/telemetry/tests/marionette/tests/functional/test_ping_server.py:14
(Diff revision 3)
> +
> +
> +class TestPingServer(TelemetryTestCase):
> +
> +    def test_ping_server_received_ping(self):
> +

nit: no need for this extra empty line.

::: toolkit/components/telemetry/tests/marionette/tests/functional/test_ping_server.py:20
(Diff revision 3)
> +        data = {'sender': 'John', 'receiver': 'Joe', 'message': 'We did it!'}
> +        headers = {'Content-type': 'application/json', 'Accept': 'text/plain'}
> +        r = requests.post(self.ping_url, data=json.dumps(data), headers=headers)
> +        assert r.status_code == 200
> +        assert len(self.ping_list) == 1
> +        assert data['sender'] == self.ping_list.pop()['sender']

So this is more like a unittest than a functional test. I would separate those into eg. `tests/marionette/tests/unit/`.
Attachment #8848582 - Flags: review?(hskupin) → review-
Comment on attachment 8848582 [details]
Bug 1301781 - Add test harness for telemetry tests

https://reviewboard.mozilla.org/r/121492/#review126474

> See above. Make use of `tempfile` instead.

I just realized that the change in the implementation of the server parsing requests made it so we don't even need a temp folder anymore. Thanks for catching that.
Comment on attachment 8848582 [details]
Bug 1301781 - Add test harness for telemetry tests

https://reviewboard.mozilla.org/r/121492/#review126474

> From reading the code I assume this is the base URL, and tests will have to add a suffix? If that is the case please rename the property appropriately.

Can you clarify on this? The tests do not have to add a separate suffix. Perhaps ping_server_url would be more appropriate?
Comment on attachment 8848582 [details]
Bug 1301781 - Add test harness for telemetry tests

https://reviewboard.mozilla.org/r/121492/#review126474

> Can you clarify on this? The tests do not have to add a separate suffix. Perhaps ping_server_url would be more appropriate?

This is just about the variable name, yes. So how can ping URLs look like? I ask because of your definition of `/pings.*`.
Comment on attachment 8848582 [details]
Bug 1301781 - Add test harness for telemetry tests

https://reviewboard.mozilla.org/r/121492/#review126474

> This is just about the variable name, yes. So how can ping URLs look like? I ask because of your definition of `/pings.*`.

Ah, that's a good catch. The url will be something like "localhost:57658/pings" So I am removing the ".*" at the end.
Comment on attachment 8848582 [details]
Bug 1301781 - Add test harness for telemetry tests

https://reviewboard.mozilla.org/r/121492/#review126474

> Given that those prefs (beside toolkit.telemetry.server) will apply to all telemetry tests you should better move everything to the harness itself and add your own default prefs there. This wouldn't require an extra restart of Firefox and saves lots of time.
> 
> See `firefox_ui_harness/runners/update.py`.

The ping server url being changed will need to trigger a restart anyways since that is only queried when firefox starts. So we may as well set the rest of the prefs here.
Comment on attachment 8848582 [details]
Bug 1301781 - Add test harness for telemetry tests

https://reviewboard.mozilla.org/r/121492/#review126474

> The ping server url being changed will need to trigger a restart anyways since that is only queried when firefox starts. So we may as well set the rest of the prefs here.

Also, I didn't create a harness class for these tests per your instructions a while back. I only created a runtests.py just running the cli command.
Comment on attachment 8848582 [details]
Bug 1301781 - Add test harness for telemetry tests

https://reviewboard.mozilla.org/r/121492/#review126474

> Also, I didn't create a harness class for these tests per your instructions a while back. I only created a runtests.py just running the cli command.

Well, it was just an idea of improvements for test runtimes. Each additional restart makes your tests slower. But it's something which could be improved later. Regarding the ping url and the HTTP server, if you don't need a fresh instance for each test, you could still run it via the harness. But again, not a blocker for this landing.
Comment on attachment 8848582 [details]
Bug 1301781 - Add test harness for telemetry tests

https://reviewboard.mozilla.org/r/121492/#review127112

r=me with the following issues fixed. Please keep in mind that I will be out the next days. So for remaining questions please contact Maja or Andreas in #ateam.

::: commit-message-34585:5
(Diff revisions 3 - 4)
> -Bug 1301781 - Submitted patch for converting ping server to marionette witha  small test to ensure that the server works.
> +Bug 1301781 - Add test harness for telemetry tests
> +
> +Added requirements.txt
> +Adjusted file structure
> +Made edits to test and variables

Those items are not pretty helpful. It would be better to have complete sentences which explain what this harness is, what it is used for, and such.

::: toolkit/components/telemetry/tests/marionette/harness/requirements.txt:3
(Diff revision 4)
> +marionette-harness
> +firefox-puppeteer
> +marionette-driver

You might want to use fixed versions for marionette related packages. See our firefox-ui tests harness, or the external media tests.

nit: please fix the ordering of entries.

::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:27
(Diff revision 4)
> +
> +    def setUp(self, *args, **kwargs):
> +        super(TelemetryTestCase, self).setUp()
> +
> +        # Start and configure server
> +        self.httpd = httpd.FixtureServer(doc_root)

The constructor if `FixtureServer` checks if the `doc_root` is a directory. Given that `www` is not part of your patch it will fail.

Also I would propose to put the `www` folder beside the `telemetry_harness` folder, and not within it.

::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:59
(Diff revision 4)
> +        self.ping_list.append(json_data)
> +        return 200
> +
> +
> +def unpack(headers, data):
> +    post_data = data

You can remove this extra variable. It has not purpose.

Something what I missed last time is that you are also using the headers. :/ So passing the request object itself into this method, might still make sense. But feel free to revert it or not.

::: toolkit/components/telemetry/tests/marionette/tests/unit/test_ping_server_received_ping.py:1
(Diff revision 4)
> +# This Source Code Form is subject to the terms of the Mozilla Public

You should also add a manifest file to this folder, which contains this single test for now.
Attachment #8848582 - Flags: review?(hskupin) → review+
Attachment #8852671 - Flags: review?(mjzffr)
Attachment #8848582 - Attachment is obsolete: true
Attachment #8852671 - Flags: review?(hskupin)
Attachment #8852671 - Flags: review?(ato)
Comment on attachment 8852671 [details]
Bug 1301781 - Add test harness for telemetry tests

https://reviewboard.mozilla.org/r/124854/#review127626

I’m not a topic expert on this, so I have just made some general comments about style.  Please take my review with a grain of salt.

Generally this patch looks clean and good.  I’m not a huge fan of the unittest-based testing style, so part of me wish we had a better story to ensure new Marionette tests would use something else.

::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/runtests.py:9
(Diff revision 2)
> +def cli():
> +    mn_cli(testcase_class=TelemetryTestCase)

I don’t understand the purpose of this function.

If you need a runtests.py file to kick off the tests, it should be made executable (`chmod u+x runtests.py`).  However, did you consider integrating this with mach?

::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:27
(Diff revision 2)
> +        self.httpd = httpd.FixtureServer(doc_root)
> +        self.httpd.start()
> +        ping_route = [("POST", re.compile('/pings'), self.pings)]
> +        self.httpd.routes.extend(ping_route)

It may work fine to add routes after the FixtureServer has been started, but I would flip these around to be on the safe side, and do self.httpd.start() after you have extended the routes.

::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:59
(Diff revision 2)
> +    return zlib.decompress(data, zlib.MAX_WBITS | 16) \
> +        if "Content-Encoding" in headers \
> +           and headers["Content-Encoding"] == "gzip" else data

Because the statement goes over three lines, I think this is more readable:

> if headers.get("Content-Encoding") == "gzip":
>     return zlib.decompress(data, zlib.MAX_WBITS | 16)
> else:
>     return data

You can likely also use dict.get("foo") which returns None if key "foo" isn’t found.

::: toolkit/components/telemetry/tests/marionette/tests/unit/test_ping_server_received_ping.py:17
(Diff revision 2)
> +
> +    def test_ping_server_received_ping(self):
> +
> +        data = {'sender': 'John', 'receiver': 'Joe', 'message': 'We did it!'}
> +        headers = {'Content-type': 'application/json', 'Accept': 'text/plain'}
> +        r = requests.post(self.ping_server_url, data=json.dumps(data), headers=headers)

‘r’ is a keyword in pdb.  Consider using a name that doesn’t conflict when using a debugger.
Attachment #8852671 - Flags: review?(ato) → review+
Comment on attachment 8852671 [details]
Bug 1301781 - Add test harness for telemetry tests

https://reviewboard.mozilla.org/r/124854/#review127680

::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/runtests.py:9
(Diff revision 2)
> +def cli():
> +    mn_cli(testcase_class=TelemetryTestCase)

Mach command is a future issue.
Attachment #8852671 - Flags: review?(mjzffr)
Attachment #8852671 - Flags: review?(hskupin)
Attachment #8852671 - Flags: review?(mjzffr)
Attachment #8852671 - Flags: review?(hskupin)
Attachment #8852671 - Flags: review?(mjzffr)
Attachment #8852671 - Flags: review?(hskupin)
Comment on attachment 8852671 [details]
Bug 1301781 - Add test harness for telemetry tests

https://reviewboard.mozilla.org/r/121490/#review127690
Comment on attachment 8852671 [details]
Bug 1301781 - Add test harness for telemetry tests

https://reviewboard.mozilla.org/r/121490/#review127692
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 1351936
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/35277e478e45
Add test harness for telemetry tests r=ato
Attachment #8850619 - Flags: feedback?(gfritzsche)
You need to log in before you can comment on or make changes to this bug.