Migrate Ping Server to Marionette

RESOLVED FIXED

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Silne30, Assigned: Silne30)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [measurement:client])

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

2 years ago
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)

Updated

2 years ago
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.
(Assignee)

Comment 2

2 years ago
(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
(Assignee)

Comment 3

2 years ago
Created attachment 8798680 [details] [diff] [review]
patch.diff

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-
(Assignee)

Comment 5

2 years ago
Created attachment 8799037 [details] [diff] [review]
wip_patch.diff

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)
(Assignee)

Comment 6

2 years ago
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+
(Assignee)

Updated

2 years ago
Depends on: 1309318
(Assignee)

Updated

2 years ago
Blocks: 1301776
(Assignee)

Comment 8

2 years ago
Created attachment 8806374 [details] [diff] [review]
wip2_patch.diff

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").
(Assignee)

Comment 11

2 years ago
Created attachment 8821324 [details] [diff] [review]
Added_asynchronous_wait_for_test_.patch

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)
(Assignee)

Comment 12

2 years ago
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)
(Assignee)

Comment 14

2 years ago
Created attachment 8821560 [details] [diff] [review]
Updates_Per_georg.patch

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)
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 16

2 years ago
(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)
(Assignee)

Comment 17

2 years ago
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)

Comment 19

2 years ago
This bug looks related to bug 1284268.

Comment 20

2 years ago
Decompressing gzipped data can be done in memory:
  plainData = zlib.decompress(postData, zlib.MAX_WBITS | 16)

The last parameter may be optional.
(Assignee)

Comment 21

2 years ago
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
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8848582 - Flags: review?(hskupin)
(Assignee)

Updated

2 years ago
Flags: needinfo?(gfritzsche)
(Assignee)

Comment 23

2 years ago
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.
(Assignee)

Comment 24

2 years ago
Created attachment 8850619 [details]
gecko logs from marionette
Attachment #8821560 - Attachment is obsolete: true
Attachment #8850619 - Flags: feedback?(gfritzsche)
(Assignee)

Comment 25

2 years ago
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.
(Assignee)

Comment 27

2 years ago
Yes. I that will need to be updated.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 30

2 years ago
mozreview-review
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-
(Assignee)

Comment 31

2 years ago
mozreview-review-reply
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.
(Assignee)

Comment 32

2 years ago
mozreview-review-reply
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 33

2 years ago
mozreview-review-reply
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.*`.
(Assignee)

Comment 34

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Comment 36

2 years ago
mozreview-review-reply
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.
(Assignee)

Comment 37

2 years ago
mozreview-review-reply
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 38

2 years ago
mozreview-review-reply
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 39

2 years ago
mozreview-review
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8852671 - Flags: review?(mjzffr)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8848582 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8852671 - Flags: review?(hskupin)
(Assignee)

Updated

2 years ago
Attachment #8852671 - Flags: review?(ato)

Comment 43

2 years ago
mozreview-review
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+
(Assignee)

Comment 44

2 years ago
mozreview-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.
(Assignee)

Updated

2 years ago
Attachment #8852671 - Flags: review?(mjzffr)
Attachment #8852671 - Flags: review?(hskupin)
(Assignee)

Updated

2 years ago
Attachment #8852671 - Flags: review?(mjzffr)
Attachment #8852671 - Flags: review?(hskupin)
(Assignee)

Updated

2 years ago
Attachment #8852671 - Flags: review?(mjzffr)
Attachment #8852671 - Flags: review?(hskupin)
(Assignee)

Comment 45

2 years ago
mozreview-review
Comment on attachment 8852671 [details]
Bug 1301781 - Add test harness for telemetry tests

https://reviewboard.mozilla.org/r/121490/#review127690
(Assignee)

Comment 46

2 years ago
mozreview-review
Comment on attachment 8852671 [details]
Bug 1301781 - Add test harness for telemetry tests

https://reviewboard.mozilla.org/r/121490/#review127692
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Depends on: 1351936

Comment 48

2 years ago
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)
Duplicate of this bug: 1294403
You need to log in before you can comment on or make changes to this bug.