1301776 - Implement Minimal Test For Telemetry

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[John Dorlus [:Silne30]]

We need to automate a barebones test case for telemetry to ensure we have all of the correct functionality in our tools.

Example test scenario:
1.) Start Firefox
2.) Handle data policy notification bar - or override it with preferences
3.) Open new tab
4.) Perform a search from search box
5.) Perform a search from awesome bar
6.) Close tab
7.) Quit firefox (Need to be able to do this from the test)
8.) Restart firefox (Need to be able to do this from the test)
9.) Wait for ping (Need to determine how long to wait or if we can force a ping)
10.) Assert correct data in ping (Need to be able to spin up a ping server from the test/harness)
* search counts
* client id
* scalar for tabs & windows (engagement measurements)
* (session/subsession counts)
* (sessionId/subsessionId, previousSessionId/previousSubsessionId)

Comment 1

[John Dorlus [:Silne30]]

Attachment: Bug 1301776 - Added telemetry-tests command with initial telemetry test with ping server

Review commit: https://reviewboard.mozilla.org/r/107908/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107908/

Comment 2

[Alessio Placitelli [:Dexter]]

Comment on attachment 8831341 [details]
Bug 1301776 - Added telemetry-tests command with initial telemetry test with ping server

Now that Georg is back, I'm leaving the review to him (unless you have a specific question for me :-) )

Comment 3

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8831341 [details]
Bug 1301776 - Added telemetry-tests command with initial telemetry test with ping server

https://reviewboard.mozilla.org/r/107908/#review110280

Going back to the initial comment with the example test case scenario:
https://bugzilla.mozilla.org/show_bug.cgi?id=1301776#c0
Did you drop these requirements? If yes why?

Note: You should be more careful about which files you commit: gecko.log & runtests.pyc shouldn't be in the commit.

::: toolkit/components/telemetry/tests/marionette_tests/telemetry_harness/testcase.py:62
(Diff revision 1)
> +        current_num_pings = len(TelemetryTestCase.ping_list)
> +        time_for_ping = 0
> +        try:
> +            self.logger.info(self.marionette.get_pref('toolkit.telemetry.send.overrideOfficialCheck'))
> +            self._send_ping(method, ping_type)
> +            while not self.ping_list > current_num_pings:

`len(self.ping_list)`  surely?

::: toolkit/components/telemetry/tests/marionette_tests/telemetry_harness/testcase.py:65
(Diff revision 1)
> +            self.logger.info(self.marionette.get_pref('toolkit.telemetry.send.overrideOfficialCheck'))
> +            self._send_ping(method, ping_type)
> +            while not self.ping_list > current_num_pings:
> +                self.logger.info("{} seconds elasped waiting for ping...".format(time_for_ping))
> +                time.sleep(1)
> +                if time_for_ping > 59:

As discussed before, i want tests themselves to be able to:
- trigger a ping in various ways
- (possibly do some other work)
- (possibly restart Firefox)
- wait for a ping (or multiple) and get contents

I don't see how this is possible with this implementation?

::: toolkit/components/telemetry/tests/marionette_tests/telemetry_harness/testcase.py:75
(Diff revision 1)
> +        except Exception as e:
> +            self.fail('Error generating ping: ' + e.message)
> +
> +    def _send_ping(self, method, ping_type):
> +        if method == 'install':
> +            # TODO: Implement ping generation by installing addon

Are those TODOs supposed to be implemented in this bug?
If not, you should remove the comments and instead track this in other bugs.

::: toolkit/components/telemetry/tests/marionette_tests/telemetry_tests/test_telemetry_ping.py:11
(Diff revision 1)
> +
> +
> +class TestPingServer(TelemetryTestCase):
> +
> +    def test_wait_for_ping(self):
> +        self.marionette.navigate('http://www.mozilla.com')

Is this relevant to the test?

::: toolkit/components/telemetry/tests/marionette_tests/telemetry_tests/test_telemetry_ping.py:14
(Diff revision 1)
> +
> +    def test_wait_for_ping(self):
> +        self.marionette.navigate('http://www.mozilla.com')
> +        with self.marionette.using_context(self.marionette.CONTEXT_CHROME):
> +            self.browser.tabbar.open_tab()
> +            assert len(self.browser.tabbar.tabs) == 2

Is this relevant to the test?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8831341 [details]
Bug 1301776 - Added telemetry-tests command with initial telemetry test with ping server

https://reviewboard.mozilla.org/r/107908/#review110748

::: toolkit/components/telemetry/tests/marionette_tests/requirements.txt:3
(Diff revision 1)
> +firefox-puppeteer
> +marionette-client==3.2.0
> +marionette-driver==2.1.0

Very outdated. This need an update to the latest versions for both packages. Btw. client is marionette-harness now.

::: toolkit/components/telemetry/tests/marionette_tests/setup.py:21
(Diff revision 1)
> +        return f.read()
> +
> +setup(name='telemetry-tests',
> +      version=PACKAGE_VERSION,
> +      description=('A collection of Mozilla Firefox Telemetry tests run '
> +                   'with Marionette'),

This package is for the harness and not the tests.

::: toolkit/components/telemetry/tests/marionette_tests/setup.py:34
(Diff revision 1)
> +          'Topic :: Software Development :: Libraries :: Python Modules',
> +      ],
> +      keywords='mozilla',
> +      author='Firefox Test Engineering Team',
> +      author_email='[e-mail]',
> +      url='https://hg.mozilla.org/mozilla-central/toolkit/telemetry/tests',

Better you create a page on MDN with a description of the harness and tests, and reference it here.

::: toolkit/components/telemetry/tests/marionette_tests/setup.py:43
(Diff revision 1)
> +      install_requires=read('requirements.txt').splitlines(),
> +      include_package_data=True,
> +      entry_points="""
> +        [console_scripts]
> +        telemetry-tests = runtests:cli
> +    """)

I would suggest to name the directories like `toolkit/components/telemetry/tests/marionette/*` so it is in sync with other components.

And this file should be under `harness` whereby tests are under `tests`. You may want to have a look at the firefox ui tests under `testing/firefox-ui`.

::: toolkit/components/telemetry/tests/marionette_tests/telemetry_harness/testcase.py:46
(Diff revision 1)
> +            'datareporting.healthreport.uploadEnabled': True,
> +            'datareporting.policy.dataSubmissionEnabled': False,
> +            'datareporting.policy.dataSubmissionPolicyBypassNotification': True,
> +            'toolkit.telemetry.log.level':  0,
> +            'toolkit.telemetry.log.dump': True
> +        }

Again, you have a custom harness and it should take care about setting up those default preference values. That would kill the necissity of having to restart after each single test. See firefox-ui harness for update tests.

::: toolkit/components/telemetry/tests/marionette_tests/telemetry_harness/testcase.py:54
(Diff revision 1)
> +        self.marionette.enforce_gecko_prefs(telemetry_prefs)
> +
> +    def tearDown(self, *args, **kwargs):
> +        super(TelemetryTestCase, self).tearDown()
> +        self.httpd.stop()
> +        self.marionette.close()

What are you going to close here? There is no need to close the last remaining tab/window. It won't work anyway.

::: toolkit/components/telemetry/tests/marionette_tests/telemetry_harness/testcase.py:57
(Diff revision 1)
> +        super(TelemetryTestCase, self).tearDown()
> +        self.httpd.stop()
> +        self.marionette.close()
> +
> +    def trigger_ping(self, method, ping_type):
> +        current_num_pings = len(TelemetryTestCase.ping_list)

Isn't this just `self.ping_list`, or do I miss something?

::: toolkit/components/telemetry/tests/marionette_tests/telemetry_harness/testcase.py:67
(Diff revision 1)
> +            while not self.ping_list > current_num_pings:
> +                self.logger.info("{} seconds elasped waiting for ping...".format(time_for_ping))
> +                time.sleep(1)
> +                if time_for_ping > 59:
> +                    self.fail("60 second ping timeout exceeded")
> +                time_for_ping += 1

All this shoul be done via `Wait().until()`, whereby the constructor of the Wait class also expects a timeout value.

::: toolkit/components/telemetry/tests/marionette_tests/telemetry_harness/testcase.py:82
(Diff revision 1)
> +        elif method == 'pref':
> +            # TODO: Implement ping generation by enforcing a preference.
> +            pass
> +        else:
> +            self.marionette.execute_script('Cu.import("resource://gre/modules/TelemetryController.jsm", this);'
> +                                           'TelemetryController.submitExternalPing("' + ping_type + '", {});')

I actually do not see a value for this extra method. It should all be in the above method.

Also why not specifying a callback as method and let the tests do what they want? You shouldn't hard-code everything in the default testcase class.

::: toolkit/components/telemetry/tests/marionette_tests/telemetry_harness/testcase.py:90
(Diff revision 1)
> +        json_data = json.loads(TelemetryTestCase.unpack(request))
> +        TelemetryTestCase.ping_list.append(json_data)
> +        return 200
> +
> +    @staticmethod
> +    def unpack(incoming):

Make this method global in this file. I do not see why it has to be part of the testcase class.

::: toolkit/components/telemetry/tests/marionette_tests/telemetry_harness/testcase.py:97
(Diff revision 1)
> +        if "Content-Encoding" in incoming.headers and incoming.headers["Content-Encoding"] == "gzip":
> +            gz_data = post_data
> +            with open('temp.gz', 'wb') as gz_file:
> +                gz_file.write(gz_data)
> +            with gzip.open('temp.gz', 'rb') as unzipped_file:
> +                plain_data = unzipped_file.read()

As already mentioned last time and never got a reply for that, why aren't you doing this all in memory? Writing to disk to read it right after only makes it slower.

Comment 5

[John Dorlus [:Silne30]]

Hi Mark,

I am looking for the API call to be able to get this data from a ping that has been sent.

* search counts
* client id
* scalar for tabs & windows (engagement measurements)
* (session/subsession counts)
* (sessionId/subsessionId, previousSessionId/previousSubsessionId)

Comment 6

[John Dorlus [:Silne30]]

Flagged wrong person.

Comment 7

[Georg Fritzsche [:gfritzsche]]

(In reply to John Dorlus [:Silne30] from comment #5)
> Hi Mark,
> 
> I am looking for the API call to be able to get this data from a ping that
> has been sent.
> 
> * search counts
> * client id
> * scalar for tabs & windows (engagement measurements)
> * (session/subsession counts)
> * (sessionId/subsessionId, previousSessionId/previousSubsessionId)

Is this for the pings that you the ping server collects?
For those we can just get the info from the parsed JSON object.

First you will need to trigger "main" pings (e.g. with addon install/uninstall or pref changes).
Then you wait to receive the corresponding "main" ping.
In the main ping you will find the properties under the following paths:
- client id: <ping>/clientId
- search counts: <ping>/payload/keyedHistograms/SEARCH_COUNTS/<engine>/sum
- tabs & windows scalars are in: <ping>/payload/scalars & <ping>/payload/keyedScalars
  - you find the individual properties in Scalars.yaml [1]
- the other properties are accessed similarly in the payload, see the "main" ping docs [2] for details

Search counts & client id would be a good start for this bug (and show that the test approach works).
We could probably take the other ones to a follow-up bug to limit the scope here.

1: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Scalars.yaml
2: https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/data/main-ping.html

Comment 8

[Georg Fritzsche [:gfritzsche]]

FWIW, you can easily inspect how pings look in about:telemetry.
Choose "current ping data" and "raw JSON" to view the current state as it would be sent out.

Comment 9

[John Dorlus [:Silne30]]

Comment on attachment 8831341 [details]
Bug 1301776 - Added telemetry-tests command with initial telemetry test with ping server

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107908/diff/1-2/

Comment 10

[Alessio Placitelli [:Dexter]]

Comment on attachment 8831341 [details]
Bug 1301776 - Added telemetry-tests command with initial telemetry test with ping server

Clearing my r? as Georg is on this.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8831341 [details]
Bug 1301776 - Added telemetry-tests command with initial telemetry test with ping server

https://reviewboard.mozilla.org/r/107908/#review120956

As we discussed on our last meeting the next working step should be the implementation of a working ping server. And for that we have already an ongoing review on bug 1301781. I do not see why everything is getting moved to this bug now, which is about getting the first telemetry test up.

Comment 12

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8831341 [details]
Bug 1301776 - Added telemetry-tests command with initial telemetry test with ping server

https://reviewboard.mozilla.org/r/107908/#review121010

See comment 11.

Comment 13

[John Dorlus [:Silne30]]

Comment on attachment 8831341 [details]
Bug 1301776 - Added telemetry-tests command with initial telemetry test with ping server

https://reviewboard.mozilla.org/r/107908/#review120956

Henrik I submitted a patch for this bug that only addressed the working ping server. As we discussed, the telemetry tests work has become an enormous, monolithic change that seems to be unlandable in its present state. The patch submitted only addresses 1301781 (migrate ping server to marionette) and a subsequent change will be landed for the telemetry test work.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8831341 [details]
Bug 1301776 - Added telemetry-tests command with initial telemetry test with ping server

https://reviewboard.mozilla.org/r/107908/#review120956

This is still the wrong bug, John. Ping server is bug 1301781. Please move the patch.

Comment 15

[John Dorlus [:Silne30]]

Comment on attachment 8831341 [details]
Bug 1301776 - Added telemetry-tests command with initial telemetry test with ping server

https://reviewboard.mozilla.org/r/107908/#review120956

Yes. Please take a look at 1301781. The patch is there.

Comment 16

[John Dorlus [:Silne30]]

I have 3 different addons that I use to generate a ping. 

NoScript Suite (Downloaded from the marketplace)
Adblock Pro (Downloaded from the marketplace)
Test Pilot (Downloaded from testpilot.firefox.com)

The only addon that generates a ping is the test pilot addon. I run the same test separately and the test only installs one addon. The telemetry client sees the change in environment yet only testpilot generates the ping.

Comment 17

[Alessio Placitelli [:Dexter]]

(In reply to John Dorlus [:Silne30] from comment #16)
> I have 3 different addons that I use to generate a ping. 
> 
> NoScript Suite (Downloaded from the marketplace)
> Adblock Pro (Downloaded from the marketplace)
> Test Pilot (Downloaded from testpilot.firefox.com)
> 
> The only addon that generates a ping is the test pilot addon. I run the same
> test separately and the test only installs one addon. The telemetry client
> sees the change in environment yet only testpilot generates the ping.

John, as mentioned over IRC, would you mind attaching the Telemetry logs for each case? Otherwise I can't really help.

Comment 18

[John Dorlus [:Silne30]]

Attachment: test_pilot

Comment 19

[John Dorlus [:Silne30]]

Attachment: adblockpro

Comment 20

[John Dorlus [:Silne30]]

I attached the logs of two of the tests. They are the marionette logs that include the logs from Telemetry. You can just use find Telemtry and it will lead you to the telemetry lines.

Comment 21

[Alessio Placitelli [:Dexter]]

(In reply to John Dorlus [:Silne30] from comment #20)
> I attached the logs of two of the tests. They are the marionette logs that
> include the logs from Telemetry. You can just use find Telemtry and it will
> lead you to the telemetry lines.

As discussed over IRC, for the AdBlock case you're hitting throttling for the environment changes [1], so no "main" ping with reason "environment-changed" gets generated.
For the "Test Pilot" addon, you're receiving a ping, but it's not a "main" ping. It's a "testpilot" ping that gets generated from a different system in Firefox, and that's probably not what you're looking for in this test.

You can probably act on the minimum subsession length "toolkit.telemetry.minSubsessionLength" [2] preference to get past this, by setting it to 0. But if that's ok or not, pretty much depends on the test you're writing.

[1] - http://searchfox.org/mozilla-central/rev/c48398abd9f0f074c69f2223260939e30e8f99a8/toolkit/components/telemetry/TelemetrySession.jsm#2093
[2] - https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/internals/preferences.html

Comment 22

[John Dorlus [:Silne30]]

I am still having this issue though I am enforcing the pref in the marionette harness.I was talking to :chutten and he informed me that the pref is queried once. I have set it to 0 and 1 but I still get the error that throttling has occurred. It happens and says last change was 8s ago.

Comment 23

[Alessio Placitelli [:Dexter]]

(In reply to John Dorlus [:Silne30] from comment #22)
> I am still having this issue though I am enforcing the pref in the
> marionette harness.I was talking to :chutten and he informed me that the
> pref is queried once. I have set it to 0 and 1 but I still get the error
> that throttling has occurred. It happens and says last change was 8s ago.

This shouldn't be a problem if you're setting the prefs before the browsing session starts, as you do with the other prefs. However, since you filed bug 1350428, let's take this conversation, there.

Comment 24

[John Dorlus [:Silne30]]

We addressed the throttling issue but have come across an issue in marionette that throws an error when trying to close tabs. This bug depends on 1351940

Comment 25

[John Dorlus [:Silne30]]

The restart issue that was going on has also been addressed. This is almost complete. There is some flakiness present i the test that needs to be addressed.

Comment 26

[John Dorlus [:Silne30]]

This script requires changing the default search engine to mozsearch as there have been some issues with working with Yahoo for the test case.

Comment 27

[John Dorlus [:Silne30]]

This test is almost finished. I have a few elements I am having some difficulty interacting with but once solved , I will be able to push this test to try.

Comment 28

[John Dorlus [:Silne30]]

Created a bug for the issue of interacting with the alert to install a search engine. I am looking for a workaround to install ehe engine on the back end. 

Either by invoking the search service: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#4042

Or by using something like the SJS files that the devs use: https://dxr.mozilla.org/mozilla-central/search?q=path:search%20file%3A.sjs&=mozilla-central

Comment 29

[John Dorlus [:Silne30]]

I am modifying this test case since the search bar is going to be removed soon.

Comment 30

[John Dorlus [:Silne30]]

Is there another step you want to replace step 4. If not, this test can be concluded as it is. The blockers for this bug are around the searchbar element which is being removed.

Comment 31

[John Dorlus [:Silne30]]

I went ahead and push the test as it is without step 4. If you are good with that, we can start the review process.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Raphael, I think this bug can be closed now, right?

Comment 33

[Raphael Aurich [:raphael] UTC+02:00]

Yes, most of the described functionality is covered by bug 1502849.

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Most, but not all? But we can still close it? So please do so.

Comment 35

[Raphael Aurich [:raphael] UTC+02:00]

We don't check scalars for windows at the moment. I'll add a check for that to the test of bug 1502849.

Comment 36

[Raphael Aurich [:raphael] UTC+02:00]

Attachment: Bug 1301776 - Verify window_open_event_count scalar data in telemetry-tests-client; r?dexter,whimboo

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c2f5fd8bef2
Verify window_open_event_count scalar data in telemetry-tests-client; r=Dexter

Comment 38

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/8c2f5fd8bef2