Closed
Bug 1485620
Opened 6 years ago
Closed 6 years ago
add gtest for PrioEncoder
Categories
(Firefox :: General, enhancement, P1)
Firefox
General
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.
Assignee | ||
Comment 1•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Assignee | ||
Comment 2•6 years ago
|
||
(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
Updated•6 years ago
|
Priority: -- → P1
Assignee | ||
Comment 3•6 years ago
|
||
(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.
Assignee | ||
Updated•6 years ago
|
Summary: add tests for PrioEncoder → add gtest for PrioEncoder
Assignee | ||
Comment 4•6 years ago
|
||
Assignee | ||
Comment 5•6 years ago
|
||
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 6•6 years ago
|
||
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 7•6 years ago
|
||
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
Assignee | ||
Comment 9•6 years ago
|
||
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)
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ed612eec41a4
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in
before you can comment on or make changes to this bug.
Description
•