Implement Deletion Ping in Telemetry Harness

ASSIGNED
Assigned to

Status

()

Toolkit
Telemetry
P4
normal
ASSIGNED
9 months ago
7 months ago

People

(Reporter: Silne30, Assigned: Silne30)

Tracking

unspecified
Points:
---

Firefox Tracking Flags

(firefox57 fix-optional)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

9 months ago
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
Priority: -- → P3
(Assignee)

Comment 1

9 months ago
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?
Flags: needinfo?(alessio.placitelli)
(Assignee)

Updated

9 months ago
Assignee: nobody → jdorlus
Status: NEW → ASSIGNED
(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?
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 3

9 months ago
Yes, I did. You helped me find that the logs were not on the right level. Thanks.
Comment hidden (mozreview-request)

Comment 5

9 months ago
mozreview-review
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.
Attachment #8891512 - Flags: review?(alessio.placitelli) → review-
(Assignee)

Comment 6

9 months ago
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 hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8891512 - Flags: review?(alessio.placitelli) → review?(mjzffr)

Comment 8

9 months ago
mozreview-review
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.
Attachment #8891512 - Flags: review?(mjzffr) → review-
Comment hidden (mozreview-request)

Comment 10

9 months ago
mozreview-review
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
Attachment #8891512 - Flags: review?(mjzffr) → review+
status-firefox57: --- → fix-optional
Priority: P3 → P4
You need to log in before you can comment on or make changes to this bug.