t.plan() and tests with asynchrony

[Chris-Hibbert]

I'm updating tests for some ERTP changes, and paying attention to tests that have .then() blocks that I've occasionally seen get skipped in test runs that claimed to pass. @michaelfig had suggested t.plan(), which I found to be very helpful in this situation. (t.plan(3) says to expect 3 assertions. It fails if there are more or fewer, and it doesn't require t.end().)

@erights saw that I was using t.plan() and asked why. t.plan() can be a maintenance nightmare because you have to keep changing the count if you add tests and can sometimes obscure failures if you get the count wrong and don't notice.

t.plan() works in my situation because it catches cases where a test breaks in ways that aren't explicitly tested for, causing the test to not reach later assertions that were intended to catch the misbehavior. I've seen tests where some promise never resolved, so a .then() block wasn't triggered, and the test framework dutifully reported that all the assertions it saw had passed, and called the test a success. With t.plan(), as long as the test author/maintainer looks at the output, and verifies that all the intended steps are being reached during the sample run, they can be sure that those same steps will be run later. @eright's response was that he was glad to hear a defense of t.plan(), but he was still skeptical.

I explored some ways of allowing tests to detect that all the .then clauses were exercised without using .plan, and shared the results with @erights. I tried a couple of things, including

1. creating promises at the start of the test that would be resolved in .then() branches, and validated in finally clauses
2. using boolean flags to indicate that the .then() clauses had completed. This required using async/await to ensure that t.end() wasn't called until all the branches were done.

test('issuer.splitMany good amount', async t => {
  let loopCount = 0;
  try {
    const { mint, issuer, amountMath } = produceIssuer('fungible');
    const oldPayment = mint.mintPayment(amountMath.make(100));
    const goodAmounts = Array(10).fill(amountMath.make(10));

    const checkPayments = async splitPayments => {
      for (const payment of splitPayments) {
        issuer.getAmountOf(payment).then(amount => {
          t.deepEqual(
            amount,
            amountMath.make(10),
            `split payment has right balance`,
          );
          loopCount += 1;
        });
      }
      t.rejects(
        () => issuer.getAmountOf(oldPayment),
        /payment not found/,
        `oldPayment no longer exists`,
      );
    };

    await E(issuer)
      .splitMany(oldPayment, goodAmounts)
      .then(checkPayments);
  } catch (e) {
    t.assert(false, e);
  } finally {
    t.equals(loopCount, 10, 'should loop 10 times');
    t.end();
  }
});

@erights was surprised that the example with async could work, since the async call should just return a promise. We tried various combinations of test cases that returned promises that resolved before and after calling t.ok(), (or never). It appears that tape must be explicitly checking for quiescence in order to decide to wait for Promise results to resolve. Our conclusion was that we shouldn't use the approach that required asynch, since we don't understand the intended semantics, we don't know whether it's documented, we don't know whether it's portable, etc.

So, for now, I'm going to carefully use t.plan(). Before we can ensure that tests complete all their branches using async/await, or by waiting in finally blocks for Promises to resolve, we'll need to understand tape's semantics better.

Some illustrative examples:

This waits 10 seconds, and then succeeds.

test('foo', t => {
  return new Promise(r => setTimeout(() => r(t.end()), 10000));
});

This succeeds

test('foo', t => {
  return Promise.resolve(null).then(() => { });
});

This fails

test('foo', t => {
  return new Promise(() => {});
});

This succeeds

test('foo', t => {
  return Promise.resolve(null).then(() => {
    t.end();
  });
})

[katelynsills]

If you get a rejected promise in a particular test, without catching, does it break all the subsequent tests?

[Chris-Hibbert]

There's some kind of error that breaks all subsequent tests, but I've never tracked it down. If the cause of that is unhandled rejected promises, I think we should come up with a systemic solution.

I hope the answer isn't adding try/catch/finally in all tests that use promises.

[katelynsills]

I hope the answer isn't adding try/catch/finally in all tests that use promises.

That's the answer that I was told previously. :)

[katelynsills]

Another data point - when we no longer have t.end() in the finally clause (because it was replaced by t.plan), any test statement (t.equals etc) after a thrown error within the same test function gives "test exited without ending", and any subsequent test functions are not begun.

[DavidBruant]

I don't know if it's relevant yet, but i'll share my experience with test frameworks

Over the last couple of years, i've had experience with the following JS test frameworks:

• mocha
• jest
• ava
• tape

mocha is cool, but only with additional plugins (like "chai-as-promised"). It's old and stable, with lots of plugins

jest works and is very relevant to test React components. One annoying limitation is that it's not possible to add per-assertion message. As far as i'm concerned, this is too limiting for my usage of tests

I've discovered ava recently. It run tests in different test files in parallel and tests within the same file concurrently (!) (so attention need to be paid on tests not sharing state... which i believe is a good thing). It has built-in promise support

tape is... minimalistic. This is an excellent feature to get started with tests, but i believe lots of agoric-sdk packages have needs that are beyond what tape provides

[Chris-Hibbert]

Here's an example from ERTP tests:

test('issuer.combine array of promises', t => {
  try {
    const { mint, issuer, amountMath } = produceIssuer('fungible');
    const paymentsP = [];
    for (let i = 0; i < 100; i += 1) {
      const freshPayment = mint.mintPayment(amountMath.make(1));
      const paymentP = issuer.claim(freshPayment);
      paymentsP.push(paymentP);
    }

    E(issuer)
      .combine(paymentsP)
      .catch(e => t.assert(false, e))
      .finally(_ => t.end());
  } catch (e) {
    t.assert(false, e);
  } finally {
    t.end();
  }
});

The return value of .combine() wasn't being checked. It was only making sure that no errors were being thrown. I want it to check the result, so I add

  const checkCombinedResult = paymentP => {
    issuer.getAmountOf(paymentP).then(pAmount => {
      t.equals(pAmount.extent, 100);
    });
  };

and then add .then(checkCombinedResult) before the .catch() clause.

The test passes fine, but it's now fragile. A mistake that I often make is getting the wrong payment or something that isn't a payment and attempting to use it. Here, if I change getAmountOf(paymentP) to getAmountOf(paymentP.foo) to show the consequences, the test silently passes. The error shows up in the log if you know what to look for, but the test is counted as passing. Since these errors are intentionally provoked some of the time, noticing the problem is hard.

If the test was preceded by t.plan(1), it catches that error, but causes following tests to also fail.

If checkCombinedResult has a catch clause for its .then, it catches the error and doesn't fail following tests. This seems a less natural coding style to me.

    const checkCombinedResult = paymentP => {
      issuer
        .getAmountOf(paymentP.foo)
        .then(pAmount => {
          t.equals(pAmount.extent, 100);
        })
        .catch(e => t.assert(false, e));
    };

In the cases I've looked at, I haven't found any situations where adding

 } finally {
    t.end();
 }

has any affect other than hiding some failures. The try/catch/finally don't catch the kinds of failures that I'm interested in.

[Chris-Hibbert]

I realized last night that I wrote all of this in the context of tests, but what I'm really talking about is when the bugs and relevant .then() clauses are in the code being tested.

It's not just uncaught rejected promises in tests that break subsequent tests, it's uncaught rejected promises in the code, so adding .catch() clauses in the tests doesn't fix the problem if our style is to not include them pervasively in the code. That's the context for my comment about .catch():

This seems a less natural coding style to me.

[Chris-Hibbert]

I looked through the code base, and didn't find any other swingsetTests that used try/catch/finally. The unit tests we have are all single vat synchronous tests, so they don't have this issue.

[dckc]

For the purposes of ava-xs (#370) it's not sufficiently clear what the resolution of this issue was. "async tests should use t.plan() along with awaiting all relevant promises" would suffice for #370 , but it's not clear that we've agreed to this. @katelynsills agreed that further discussion is in order.

note especially the case of test-barter #370 (comment)

[dckc]

ah... I see "t.plan() is still occasionally useful, but only if your assertion is being run in a callback or then, and you can't find a way to rewrite it to do the assertion at the top level of the function instead" -- https://github.com/Agoric/agoric-sdk/wiki/agoric-sdk-unit-testing

Perhaps that suffices and I can close this back up? I'll stand by for a bit of feedback.

cc @dtribble

[dckc]

@dtribble notes ava documents when to use plan. That documentation seems to suffice for all the purposes I can see.