Closed Bug 1485620 Opened 2 years ago Closed 2 years ago

add gtest for PrioEncoder

Categories

(Firefox :: General, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 64
Tracking Status
firefox64 --- fixed

People

(Reporter: rhelmer, Assigned: rhelmer)

References

Details

Attachments

(1 file)

Bug 1421501 introduced a simple Prio encoder for Firefox intended for use by Telemetry. Prio is privacy-preserving system for the collection of aggregate statistics.

https://github.com/mozilla/libprio/tree/master/browser-test currently contains a test that starts xpcshell from C code, and generates a public+private keypair and uses libprio to verify that the PrioEncoder JS code is working as intended.

However this doesn't run as part of Firefox CI and is a bit involved, so in this bug let's get that ported over so it can be run as a unit test.

The most straightforward way is probably to add some test-only methods to the PrioEncoder DOM, and to drive the whole thing using xpcshell.
After playing with this locally some, we're going to need to pull in the latest libprio for https://github.com/mozilla/libprio/pull/14 because debug builds have an assert if there are any NSS resources not freed at app shutdown time.
Keywords: leave-open
(In reply to Robert Helmer [:rhelmer] from comment #1)
> After playing with this locally some, we're going to need to pull in the
> latest libprio for https://github.com/mozilla/libprio/pull/14 because debug
> builds have an assert if there are any NSS resources not freed at app
> shutdown time.

Actually we ended up vendoring this fix as part of some other post-autoland bustage, so we should be good here.
Keywords: leave-open
(In reply to Robert Helmer [:rhelmer] from comment #0)
> The most straightforward way is probably to add some test-only methods to
> the PrioEncoder DOM, and to drive the whole thing using xpcshell.

Re-thinking this a bit... I think it'd actually be cleaner to just turn https://github.com/mozilla/libprio/blob/master/browser-test/main.c into a gtest, along with a bunch of other checks that invalid argument, public keys, etc. don't cause problems.

I can't think of any particular advantage to testing this from JS with xpcshell since none of the implementation is in JS, we should be able to test this from the C++ DOM side.
Summary: add tests for PrioEncoder → add gtest for PrioEncoder
Blocks: 1491737
Comment on attachment 9009646 [details]
Bug 1485620 - add gtest for PrioEncoder r?hsivonen

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a95ecb2483d471d4a59fa3b0bbee1757f106608e

Henry I don't think I can ask you for review in Phabricator but would you mind taking a look at this patch too?

This is https://github.com/mozilla/libprio/blob/master/browser-test/main.c rewritten as a googletest, using the JS API to call the PrioEncoder.encode function. Thanks!
Attachment #9009646 - Flags: review?(henrycg-bugzilla)
Comment on attachment 9009646 [details]
Bug 1485620 - add gtest for PrioEncoder r?hsivonen

Henri Sivonen (:hsivonen) has approved the revision.
Attachment #9009646 - Flags: review+
Comment on attachment 9009646 [details]
Bug 1485620 - add gtest for PrioEncoder r?hsivonen

Bobby Holley (:bholley) has approved the revision.
Attachment #9009646 - Flags: review+
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ed612eec41a4
add gtest for PrioEncoder r=bholley,hsivonen
Comment on attachment 9009646 [details]
Bug 1485620 - add gtest for PrioEncoder r?hsivonen

Henry, don't worry about reviewing this here - I'll remove the upstream test in a PR to github.com/mozilla/libprio and we can discuss whether this gtest needs any changes there. Thanks!
Attachment #9009646 - Flags: review?(henrycg-bugzilla)
https://hg.mozilla.org/mozilla-central/rev/ed612eec41a4
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in before you can comment on or make changes to this bug.