1518152 - Create a TelemetryTestUtils.jsm and add some useful functions to it

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement)

Description

[Mark Banner (:standard8)]

In bug 1510570, I wish to move a test for some code that interacts with Telemetry from one directory to another.

However, there's a few helper functions that I don't want to duplicate (some even more than they are already), an therefore I was thinking it would be helpful to have a TelemetryTestUtils.jsm where we could collect all of these.

I've a work in progress that I'll attach in a moment to get some feedback on the approach I'm proposing.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1518152 - Create a TelemetryTestUtils.jsm and start using it in browser/modules/test. r?dexter

Comment 2

[Mark Banner (:standard8)]

Alessio, Chris, if you could give me feedback on the attached patch, that'd be great. Feel free to redirect if I'm asking the wrong people.

If there's not too much work, I'm happy to push it forward.

Comment 3

[Chris H-C :chutten]

I have some feelings about how some of the utility functions are implemented (the canRecordExtended gymnastics for getParentProcessScalars is... well, I know why it's there but I'm unhappy about that and I think we should be able to fix it with some clever defaults), but seeing as this is a rearrangement I'm going to just grumble about it :)

Having a telemetry test utilites module is an excellent idea. It'll help us with our test documentation efforts, it'll make used-only-in-test behaviours easier to isolate and improve, and it should reduce the number of times people need to write the same boilerplate to do what should be simple.

My only comment at this point is that it's in the bare telemetry/tests/ folder instead of in a harness-specific (or, if it's cross-harness, some utils/) folder.

Comment 4

[Alessio Placitelli [:Dexter]]

(In reply to Mark Banner (:standard8) from comment #2)

Alessio, Chris, if you could give me feedback on the attached patch, that'd
be great. Feel free to redirect if I'm asking the wrong people.

Thank you for doing this, it's a great idea! I stand by what Chris said. I left some additional comments on Phabricator.

Comment 5

[Mark Banner (:standard8)]

(In reply to Chris H-C :chutten from comment #3)

I have some feelings about how some of the utility functions are implemented (the canRecordExtended gymnastics for getParentProcessScalars is... well, I know why it's there but I'm unhappy about that and I think we should be able to fix it with some clever defaults), but seeing as this is a rearrangement I'm going to just grumble about it :)

Yeah, I definitely agree they aren't perfect. I did think about rewriting them a little, then got into a state where I realised I was over engineering them. My hope is that by having them in a central place they can be improved over time.

My only comment at this point is that it's in the bare telemetry/tests/ folder instead of in a harness-specific (or, if it's cross-harness, some utils/) folder.

It will be available to mochitest and xpcshell-test test suites. So I'll go with adding a utils folder for now.

Regarding migrating other tests, I'll do the rest of browser/modules/test/browser/ in this bug, but will file a follow-up for other places.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f5e0f6f9b55
Create a TelemetryTestUtils.jsm and start using it in browser/modules/test. r=Dexter

Comment 7

[Mark Banner (:standard8)]

I filed bug 1518756 for spreading this out to more places.

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5f5e0f6f9b55