1382821 - Implement Deletion Ping in Telemetry Harness

Status: NEW

(Toolkit :: Telemetry, enhancement, P4)

Description

[John Dorlus [:Silne30]]

Scenario #1 
Start browser
Verify that datareporting.healthreport.uploadEnabled=True (turns off FHR (Firefox Health Report) upload: datareporting.healthreport.uploadEnabled=False)
Wait for deletion ping

Comment 1

[John Dorlus [:Silne30]]

Just turning off this pref triggers an environment-change ping to be sent but not a deletion ping. In the docs, it mentions that the user turns off FHR from the preferences panel which in turn sets the uploadEnabled pref to False. Does changing this pref on the back end not achieve the same result. Is this somehow tied to the actual switch being triggered in the UI?

Comment 2

[Alessio Placitelli [:Dexter]]

(In reply to John Dorlus [:Silne30] from comment #1)
> Just turning off this pref triggers an environment-change ping to be sent
> but not a deletion ping. In the docs, it mentions that the user turns off
> FHR from the preferences panel which in turn sets the uploadEnabled pref to
> False. Does changing this pref on the back end not achieve the same result.
> Is this somehow tied to the actual switch being triggered in the UI?

No, that's not tied to the UI, it's watching the preference itself.
I've just tested this locally and it works as expected. Moreover, no XPCSHELL test is failing, so I'm confident we are not regressing there.

Could you please provide a trace of the telemetry logs since the test starts? Did you try this manually, outside of the harness?

Comment 3

[John Dorlus [:Silne30]]

Yes, I did. You helped me find that the logs were not on the right level. Thanks.

Comment 4

[John Dorlus [:Silne30]]

Attachment: Bug 1382821 - Deletion Ping Received Test

Added test_deletion_ping_received
Added manifest file entry
Made change to testcase.py to allow any pref to be toggled.

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

Comment 5

[Alessio Placitelli [:Dexter]]

Comment on attachment 8891512 [details]
Bug 1382821 - Deletion Ping Received Test

https://reviewboard.mozilla.org/r/162640/#review168230

::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:73
(Diff revision 1)
>          # assert len(self.ping_list) == 1
>          return self.ping_list.pop()
>  
> -    def toggle_update_pref(self):
> -        value = self.marionette.get_pref('app.update.enabled')
> -        self.marionette.enforce_gecko_prefs({'app.update.enabled': not value})
> +    def toggle_pref(self, pref):
> +        value = self.marionette.get_pref(pref)
> +        self.marionette.set_pref(pref, not value)

I'm not really the right person to review this code, I think this should be reviewed by someone that knows more about the test harness, not us from Telemetry.

However, why did you switch from |enforce_gecko_prefs| to |set_pref|?

::: toolkit/components/telemetry/tests/marionette/tests/client/manifest.ini:5
(Diff revision 1)
>  [DEFAULT]
>  tags = client
>  
>  [test_main_tab_scalars.py]
> +

nit: no need to add a blank line here

::: toolkit/components/telemetry/tests/marionette/tests/client/test_deletion_ping_received.py:5
(Diff revision 1)
> +# 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/.
> +
> +# from marionette_driver.wait import Wait

Why is this commented? Please delete it if it's not being used.

Comment 6

[John Dorlus [:Silne30]]

Thanks. I switched from enforce_gecko_prefs to set_pref because enforce_gecko_pref causes a browser restart. https://marionette-client.readthedocs.io/en/master/reference.html#marionette_driver.marionette.Marionette.enforce_gecko_prefs

Addressing other nits.

Comment 7

[John Dorlus [:Silne30]]

Comment on attachment 8891512 [details]
Bug 1382821 - Deletion Ping Received Test

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

Comment 8

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8891512 [details]
Bug 1382821 - Deletion Ping Received Test

https://reviewboard.mozilla.org/r/162640/#review168392

::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:54
(Diff revision 2)
>      def wait_for_ping(self, action_func, ping_filter_func):
>          current_num_pings = len(self.ping_list)
>          if callable(action_func):
>              action_func()
>          try:
>              Wait(self.marionette, 60).until(lambda _: len(self.ping_list) > current_num_pings)
>          except Exception as e:
>              self.fail('Error generating ping: {}'.format(e.message))
>          # Filter pings based on type and reason to make sure right ping is captured.
>          self.ping_list = [p for p in self.ping_list if ping_filter_func(p)]

As a side note, given the way `action_func` is used here, you can make it an optional argument with default value None. 

```
def wait_for_ping(self, ping_func, action_func=None)
```

Also, should it even be okay pass a non-callable as action_func? Rather than silently not calling it, it might make future debugging easier to just let an Exception occur.

::: toolkit/components/telemetry/tests/marionette/tests/client/test_deletion_ping_received.py:11
(Diff revision 2)
> +
> +
> +class TestDeletionPingReceived(TelemetryTestCase):
> +
> +    def test_deletion_ping_received(self):
> +        action_func = self.toggle_pref('datareporting.healthreport.uploadEnabled')

`toggle_pref` always returns `None`, so I don't understand why it's being assigned to `action_func` or being passed to `wait_for_ping`. Wouldn't it make more sense to pass `None` to `wait_for_ping`?

::: toolkit/components/telemetry/tests/marionette/tests/client/test_deletion_ping_received.py:17
(Diff revision 2)
> +        ping = self.wait_for_ping(action_func, lambda p: p['type'] == 'deletion')
> +
> +        assert self.client_id == ping['clientId']
> +        assert ping['type'] == 'deletion'
> +        assert ping['payload'] == {}
> +        self.logger.info(ping)

It would be helpful to move the logging to before the asserts, so you have some info in the logs before a test failure.

Comment 9

[John Dorlus [:Silne30]]

Comment on attachment 8891512 [details]
Bug 1382821 - Deletion Ping Received Test

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

Comment 10

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8891512 [details]
Bug 1382821 - Deletion Ping Received Test

https://reviewboard.mozilla.org/r/162640/#review168886

r+wc A few style nits up for your consideration. Land as you see fit.

::: commit-message-16ffc:1
(Diff revision 3)
> +Bug 1382821 - Deletion Ping Received Test

Nit: I usually try to word my commit message as an action like: "Test that deletion ping is received"

::: commit-message-16ffc:3
(Diff revision 3)
> +Bug 1382821 - Deletion Ping Received Test
> +
> +Added test_deletion_ping_received

Nit: I wouldn't usually itemize simple changes like this in the second part of a commit message. Instead I usually summarize why the overall change is needed (if it's not immediately obvious from the summary) and/or explain why I'm choosing a particular solution.

::: toolkit/components/telemetry/tests/marionette/tests/client/test_deletion_ping_received.py:11
(Diff revision 3)
> +
> +
> +class TestDeletionPingReceived(TelemetryTestCase):
> +
> +    def test_deletion_ping_received(self):
> +

Nit: remove blank line