Address review comments

Author: Dexterp37

glean/src/core/pings/ping_type.ts

@@ -27,7 +27,7 @@ class PingType implements CommonPingData {
   // execute and synchronize with the submission API.
   private resolveTestPromiseFunction?: PromiseCallback;
   private rejectTestPromiseFunction?: PromiseCallback;
-  private testValidator?: ValidatorFunction;
+  private testCallback?: ValidatorFunction;
 
   constructor (meta: CommonPingData) {
     this.name = meta.name;
@@ -59,69 +59,55 @@ class PingType implements CommonPingData {
     // other dispatched tasks. Unfortunately, this causes a deadlock.
     // In order to work around that problem, we kick off validation
     // right before the actual submission takes place, through another
-    // async function (and not through the dispatcher). We then await
-    // in the dispatched submission task on the related promise, which
-    // will always resolve due to the use of `finally`.
-    if (this.testValidator) {
-      const cleanup = () => {
-        this.resolveTestPromiseFunction = undefined;
-        this.rejectTestPromiseFunction = undefined;
-        this.testValidator = undefined;
-      };
-
-      this.testValidator(reason)
+    // async function (and not through the dispatcher). After validation
+    // is complete, regardless of the outcome, the ping submission is
+    // finally triggered.
+    if (this.testCallback) {
+      this.testCallback(reason)
         .then(() => {
-          // Temporarily store the function and then clean up, so that fast consecutive
-          // calls to `testBeforeNextSubmit` don't fail because things are still set
-          // after the calling promise is resolved.
-          const resolver = this.resolveTestPromiseFunction;
-          cleanup();
-
-          this.internalSubmit(reason, resolver);
+          PingType._private_internalSubmit(this, reason, this.resolveTestPromiseFunction);
         })
         .catch(e => {
           console.error(`There was an error validating "${this.name}" (${reason ?? "no reason"}):`, e);
-
-          // Temporarily store the function and then clean up, so that fast consecutive
-          // calls to `testBeforeNextSubmit` don't fail because things are still set
-          // after the calling promise is resolved.
-          const rejecter = this.rejectTestPromiseFunction;
-          cleanup();
-
-          this.internalSubmit(reason, rejecter);
+          PingType._private_internalSubmit(this, reason, this.rejectTestPromiseFunction);
         });
     } else {
-      this.internalSubmit(reason);
+      PingType._private_internalSubmit(this, reason);
     }
   }
 
-  private internalSubmit(reason?: string, testResolver?: PromiseCallback): void {
+  private static _private_internalSubmit(instance: PingType, reason?: string, testResolver?: PromiseCallback): void {
     Context.dispatcher.launch(async () => {
       if (!Context.initialized) {
         console.info("Glean must be initialized before submitting pings.");
         return;
       }
 
-      if (!Context.uploadEnabled && !this.isDeletionRequest()) {
+      if (!Context.uploadEnabled && !instance.isDeletionRequest()) {
         console.info("Glean disabled: not submitting pings. Glean may still submit the deletion-request ping.");
         return;
       }
 
       let correctedReason = reason;
-      if (reason && !this.reasonCodes.includes(reason)) {
+      if (reason && !instance.reasonCodes.includes(reason)) {
         console.error(`Invalid reason code ${reason} from ${this.name}. Ignoring.`);
         correctedReason = undefined;
       }
 
       const identifier = generateUUIDv4();
-      await collectAndStorePing(identifier, this, correctedReason);
+      await collectAndStorePing(identifier, instance, correctedReason);
 
       // 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.
       if (testResolver) {
         testResolver();
+
+        // Finally clean up!
+        instance.resolveTestPromiseFunction = undefined;
+        instance.rejectTestPromiseFunction = undefined;
+        instance.testCallback = undefined;
       }
     });
   }
@@ -133,22 +119,22 @@ class PingType implements CommonPingData {
    *
    * TODO: Only allow this function to be called on test mode (depends on Bug 1682771).
    *
-   * @param validatorFn The asynchronous validation function to run in order to validate
-   *                    the ping content.
+   * @param callbackFn The asynchronous validation function to run in order to validate
+   *                   the ping content.
    *
    * @returns A `Promise` resolved when the ping is collected and the validation function
    *          is executed.
    */
-  async testBeforeNextSubmit(validatorFn: ValidatorFunction): Promise<void> {
-    if (this.testValidator) {
+  async testBeforeNextSubmit(callbackFn: ValidatorFunction): Promise<void> {
+    if (this.testCallback) {
       console.error(`There is an existing test call for ping "${this.name}". Ignoring.`);
       return;
     }
 
     return new Promise((resolve, reject) => {
       this.resolveTestPromiseFunction = resolve;
       this.rejectTestPromiseFunction = reject;
-      this.testValidator = validatorFn;
+      this.testCallback = callbackFn;
     });
   }
 }

glean/tests/core/pings/index.spec.ts

@@ -118,7 +118,8 @@ describe("PingType", function() {
 
     // We did not call the testing API yet, internals should be undefined.
     assert.strictEqual(ping["resolveTestPromiseFunction"], undefined);
-    assert.strictEqual(ping["testValidator"], undefined);
+    assert.strictEqual(ping["rejectTestPromiseFunction"], undefined);
+    assert.strictEqual(ping["testCallback"], undefined);
 
     let validatorRun = false;
     const p = ping.testBeforeNextSubmit(r => {
@@ -129,7 +130,8 @@ describe("PingType", function() {
 
     // Internals should be defined after the API was called.
     assert.notStrictEqual(ping["resolveTestPromiseFunction"], undefined);
-    assert.notStrictEqual(ping["testValidator"], undefined);
+    assert.notStrictEqual(ping["rejectTestPromiseFunction"], undefined);
+    assert.notStrictEqual(ping["testCallback"], undefined);
 
     ping.submit("test");
     await p;
@@ -202,21 +204,21 @@ describe("PingType", function() {
     }
   });
 
-  it("runs a validator multiple times fails when not awaiting", function() {
+  it("running a validator multiple times fails when not awaiting", function() {
     const ping = new PingType({
       name: "custom",
       includeClientId: true,
       sendIfEmpty: false
     });
 
-    assert.strictEqual(ping["testValidator"], undefined);
+    assert.strictEqual(ping["testCallback"], undefined);
 
     const testFunction = async () => Promise.resolve();
     void ping.testBeforeNextSubmit(testFunction);
-    assert.strictEqual(ping["testValidator"], testFunction);
+    assert.strictEqual(ping["testCallback"], testFunction);
 
     void ping.testBeforeNextSubmit(() => Promise.resolve());
-    assert.strictEqual(ping["testValidator"], testFunction);
+    assert.strictEqual(ping["testCallback"], testFunction);
   });
 
   it("runs a validator that rejects", async function() {
@@ -259,9 +261,13 @@ describe("PingType", function() {
     const TEST_NUM_ADDITIONS = 100;
 
     const p = ping.testBeforeNextSubmit(async () => {
+      // The timeout is here to make the test fail more
+      // reliably with the current implementation.
       await new Promise(r => setTimeout(r, 100));
 
       const value = await counter.testGetValue();
+      // We don't expect any value to be stored, because the
+      // calls to 'add' happen after the ping submission.
       assert.strictEqual(value, undefined);
       validatorRun = true;
     });
@@ -288,7 +294,7 @@ describe("PingType", function() {
       reasonCodes: ["test"]
     });
 
-    const counter = new CounterMetricType({
+    const delayedCounter = new CounterMetricType({
       category: "aCategory",
       name: "aCounterMetric",
       sendInPings: ["custom"],
@@ -310,8 +316,8 @@ describe("PingType", function() {
     const p = ping.testBeforeNextSubmit(async () => {
       await new Promise(r => setTimeout(r, 100));
 
-      assert.strictEqual(await counter.testGetValue(), 37, "Canary must match");
-      const value = await counter.testGetValue();
+      assert.strictEqual(await canary.testGetValue(), 37, "Canary must match");
+      const value = await delayedCounter.testGetValue();
       assert.strictEqual(value, undefined);
       validatorRun = true;
     });
@@ -325,10 +331,9 @@ describe("PingType", function() {
 
     await p;
     for (let i = 0; i < TEST_NUM_ADDITIONS; i++) {
-      counter.add();
+      delayedCounter.add();
     }
 
-
     assert.ok(validatorRun);
   });
 });