Bug 1701889 - Implement the ping testing api #202
Conversation
Dexterp37
force-pushed
the
ping_testing_api
branch
from
ac5f717 to
eac5ffc
Compare
brizental
left a comment
brizental
left a comment
There was a problem hiding this comment.
I am ok with the overall approach. I don't see any other way to implement this feature either. I'll move Bug 1682771 to P2 though. Disallowing test APIs outside of tests seems even more important with this feature.
Dexterp37
force-pushed
the
ping_testing_api
branch
2 times, most recently
from
2ae611a to
199583e
Compare
brizental
left a comment
brizental
left a comment
There was a problem hiding this comment.
Will continue on the tests I was doing yesterday, my concerns still remain. I explained what are these concerns on the comment I left on the tests code.
|
Ad discussed with @brizental , sadly we have no way to guarantee that metric calls executed after Sadly, that's a bigger change. I'm closing this PR. |
|
Let's give this another opportunity. |
Dexterp37
force-pushed
the
ping_testing_api
branch
from
35ffbde to
1a7ce5e
Compare
This commit adds testBeforeNextSubmit to execute a validation function synchronously before the target ping is collected.
... of this implementation
Dexterp37
force-pushed
the
ping_testing_api
branch
from
1a7ce5e to
991d3fc
Compare
glean/src/core/pings/ping_type.ts
Outdated
| // This guarantees that, when running tests, the promise returned by | ||
| // `testBeforeNextSubmit` is resolved after the ping is collected: this is | ||
| // needed to make sure calling the testing APIs on metrics behave consistently | ||
| // if tests run fast. |
There was a problem hiding this comment.
Again, I think this comment is a bit confusing and the code speaks for itself.
There was a problem hiding this comment.
Still think this comment is not necessary... But also wrong now? Specifically
// [...] this is
// needed to make sure calling the testing APIs on metrics behave consistently
// if tests run fast.
17e4992 to
15a6526
Compare
glean/src/core/pings/ping_type.ts
Outdated
| // This guarantees that, when running tests, the promise returned by | ||
| // `testBeforeNextSubmit` is resolved after the ping is collected: this is | ||
| // needed to make sure calling the testing APIs on metrics behave consistently | ||
| // if tests run fast. |
There was a problem hiding this comment.
Still think this comment is not necessary... But also wrong now? Specifically
// [...] this is
// needed to make sure calling the testing APIs on metrics behave consistently
// if tests run fast.
glean/src/core/pings/ping_type.ts
Outdated
| * TODO: Only allow this function to be called on test mode (depends on Bug 1682771). | ||
| * | ||
| * @param callbackFn The asynchronous validation function to run in order to validate | ||
| * the ping content. |
There was a problem hiding this comment.
nit: Align the second line with callbackFn.
| sendIfEmpty: false, | ||
| }); | ||
| await submitSync(ping); | ||
| const storedPings = await Context.pingsDatabase["store"]._getWholeStore(); |
There was a problem hiding this comment.
nit: can you rename this file to ping_type.spec.ts since we are here?
Dexterp37
force-pushed
the
ping_testing_api
branch
from
15a6526 to
31051c4
Compare
2 participants
This implements the pings testing API, as discussed and designed in the related bug.