Closed Bug 1527299 Opened 2 years ago Closed 2 years ago

Make it easier to test events through TelemetryTestUtils

Categories

(Toolkit :: Telemetry, enhancement, P1)

enhancement
Points:
3

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: gfritzsche, Assigned: chutten)

Details

Attachments

(5 files, 3 obsolete files)

Improve Telemetry events testing API

This is just a patch i tested locally that the first patch is based on.

I was looking through our events usage in Firefox, including how we test them.
I found that it takes too much knowledge and two extra steps on the testing engineers side to set things up.
So this is sketching out an idea to make the event testing in TelemetryTestUtils easier to use.

This might be nice to support the upcoming Firefox event instrumentations (given us a concrete motivation and use-case).

Summary: Bug xxx - Make it easier to test events through TelemetryTestUtils → Make it easier to test events through TelemetryTestUtils
Priority: -- → P1

Step 1 is to research devtools', Usage Telemetry's, telemetry xpcshell tests' testing of Events for ideas for API shapes.

Assignee: nobody → chutten
Status: NEW → ASSIGNED
Points: --- → 3

Research:

devtools:

  • documents how to test Event Telemetry. (Which means if we make this form of test easier we may wish to update the docs)
  • Tests are most often of the form "filter to only records that match these category,method,object tuples, and ensure they have the exact composition and order in [EXPECTED_ARRAY]"
  • They have their own helper for checking a solitary event, allowing a pass if the session_id matches a form.
  • Not at all interested in non-parent events
  • Likes transforming the raw event array to a struct with named members. Interesting, and much more readable.

Cert Error and Tracking UI:

  • I see some exact string matching, mostly interested in solitary events filtered by exact category, object, and method,

Browser Usage Telemetry:

  • Uses TelemetryTestUtils.assertEvents directly
  • Also checks once or twice that no events match. Interesting case.

Uptake Telemetry:

  • Also transforms the raw arrays to structs like devtools does.
  • Has its own helpers which makes a lot of sense because it's still only testing the histogram version and not the event version. I expect it'll be converted soonish.

Extensions:

  • Also has its own helpers of the form "filter to known category,method,object and check exact equality" (in this case using Assert.deepEqual.

Normandy:

And I have to go catch my bus. Tomorrow: Telemetry's own Events tests.

Telemetry:

  • Performs validation of the returned event structure, like checking the data types (we can ignore for a utils method)
  • Validates that Event Summary counts the right number of events (we can ignore for a utils method, though double-checking against Event Summaries might not be a bad idea if, for instance, we can't find an event in a snapshot)
  • Tests registration (can ignore)
  • Checks TELEMETRY_EVENT_RECORDING_ERROR... might be interesting to use this similarly to Event Summary counts.
  • When it checks events in bulk it uses raw arrays and doesn't filter
  • Tests dynamically-registered events

mozapps/extensions:

  • Like devtools and uptake filters on category,method.
  • Unlike them, it matches the raw arrays instead of structs with named fields

In summary it seems as though there is some broad demand for a Telemetry Events test utility that filters the snapshot on category,method,object (or some subset of them) and either matches the result against a list of known events (or parts of the events) or asserts that there's the right number of them (the right number might be 0).

The list of known events could be a raw array but seems to be nicer if it is a struct with named fields. The field values might be strings to check against, RegExps to exec, or functions to call with the value.

Only we (Telemetry) ever want to not clear events on snapshot. Everyone else either does it explicitly with the final true parameter or implicitly by only snapshotting once. So we can take care of that for callers, so long as we're clear about it.

Only we (Telemetry) seem to care about dataset at all, so this utility can do away with the whole optin/optout/base/extended mess and always snapshot every event for filter and comparison.

There do exist callers that want to check non-parent-process events, so we should probably oblige.

--

I think we can work with this.

It's tempting to do it all in one, where the list of expected events contains the category,method,object we'll first filter on and so on... but if so many people in so many places think about testing events as "filter first, then match" then the API should match.

Attachment #9043253 - Attachment is obsolete: true
Attachment #9043254 - Attachment is obsolete: true

This adds a general-purpose testing API for testing Telemetry Events, and a
handy extra API (assertNumberOfEventsAndClear) for if you just want to count.

Also, it turns out we no longer debug assert when recording too many events, so
we can turn on test_event_summary_limit in debug.

Depends on D20863

Attachment #9046021 - Attachment description: Bug 1527299 - Add TelemetryTestUtils.assertEventsAndClear r?janerik → Bug 1527299 - Add TelemetryTestUtils.assertEvents r?janerik
Attachment #9046022 - Attachment description: Bug 1527299 - Convert Telemetry Event tests to assertEventsAndClear r?janerik → Bug 1527299 - Convert Telemetry Event tests to assertEvents r?janerik
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/574433d7795e
Add TelemetryTestUtils.assertEvents r=janerik
https://hg.mozilla.org/integration/autoland/rev/034dce9d9393
Convert Telemetry Event tests to assertEvents r=janerik
https://hg.mozilla.org/integration/autoland/rev/30cf58a30352
Add a short explanation of Event Telemetry Testing to the docs. r=janerik
https://hg.mozilla.org/integration/autoland/rev/0b75d7939fe2
Update devtools' Event Telemetry doc and example r=miker
https://hg.mozilla.org/integration/autoland/rev/5dc14c7be626
Update callers of assertEvents to new form r=Standard8
You need to log in before you can comment on or make changes to this bug.