Test suite should capture how we expect the library to be used

[marcelomorgado]

On UX context, our users will dispatch actions that will trigger some UI response for success or failure of the processes. For now, all of our test suites wasn't written closer that way, we are using in almost all of test cases the waitForNonceToUpdate function and we expect that will not be used for our users. The way to write test cases to get closer as we expected that the code will be it to using mocha done() function. Refs: https://github.com/tasitlabs/TasitSDK/pull/207#discussion_r258306552 https://github.com/mochajs/mocha/issues/2988#issuecomment-327200194 https://mochajs.org/#asynchronous-code

[marcelomorgado]

I'll start to use done() from now for new test cases that I'll wrote.

[marcelomorgado]

Hey @pcowgill, I'm working on that on gnosis safe test suites, what do you think?

Before (with waiting)

    it("wallet owner should withdraw some ethers", async () => {
      const signers = [anaWallet];
      const { address: toAddress } = anaWallet;
      const value = ONE;

      gnosisSafe.setWallet(anaWallet);
      const execTxAction = await gnosisSafe.transferEther(
        signers,
        toAddress,
        value
      );
      await execTxAction.waitForNonceToUpdate();

      const balanceAfterWithdraw = await provider.getBalance(
        GNOSIS_SAFE_ADDRESS
      );
      expect(`${balanceAfterWithdraw}`).to.equal(`${ZERO}`);
    });

After (without waiting)

    it("wallet owner should withdraw some ethers", async () => {
      const signers = [anaWallet];
      const { address: toAddress } = anaWallet;
      const value = ONE;

      gnosisSafe.setWallet(anaWallet);
      const action = await gnosisSafe.transferEther(signers, toAddress, value);

      const onConfirmation = async message => {
        const { data } = message;
        const { args } = data;

        action.unsubscribe();

        const balance = await provider.getBalance(GNOSIS_SAFE_ADDRESS);
        expect(`${balance}`).to.equal(`${ZERO}`);
      };

      const onError = message => {
        const { error } = message;
        console.log(error);
      };

      await dispatch(action, onConfirmation, onError);
    });

    const dispatch = async (action, onConfirmation, onError) => {
      return new Promise((resolve, reject) => {
        action.on("confirmation", async message => {
          try {
            await onConfirmation(message);
            resolve();
          } catch (error) {
            reject(error);
          }
        });

        action.on("error", message => {
          const { error } = message;
          onError(message);
          reject(error);
        });
      });
    };

Note: done() demands that test case is a sync function and I'm not sure if will be good idea use promises style instead of async/await.

[pcowgill]

@marcelomorgado Does this not work?

it("wallet owner should withdraw some ethers", (done) => {
  const signers = [anaWallet];
  const { address: toAddress } = anaWallet;
  const value = ONE;

  gnosisSafe.setWallet(anaWallet);
  const action = await gnosisSafe.transferEther(signers, toAddress, value);

  const onConfirmation = async message => {
    const { data } = message;
    const { args } = data;

    action.unsubscribe();

    const balance = await provider.getBalance(GNOSIS_SAFE_ADDRESS);
    expect(`${balance}`).to.equal(`${ZERO}`);
    done();
  };

  const onError = message => {
    const { error } = message;
    console.log(error);
    done(error);
  };

  action.send();

});

[pcowgill]

@marcelomorgado Also, since we weren't handling the error case in the async/await version with a try/catch, it's not a 1-1 comparison to include the onError code in the other version

[marcelomorgado]

@marcelomorgado Does this not work?

it("wallet owner should withdraw some ethers", (done) => {
  const signers = [anaWallet];
  const { address: toAddress } = anaWallet;
  const value = ONE;

  gnosisSafe.setWallet(anaWallet);
  const action = await gnosisSafe.transferEther(signers, toAddress, value);

  const onConfirmation = async message => {
    const { data } = message;
    const { args } = data;

    action.unsubscribe();

    const balance = await provider.getBalance(GNOSIS_SAFE_ADDRESS);
    expect(`${balance}`).to.equal(`${ZERO}`);
    done();
  };

  const onError = message => {
    const { error } = message;
    console.log(error);
    done(error);
  };

  action.send();

});

No, not really because sync functions shouldn't have await.

[pcowgill]

@marcelomorgado Oh oops, I missed those await's in there - I meant to write it without any but did it too quickly.

[pcowgill]

@marcelomorgado How about this

it("wallet owner should withdraw some ethers", (done) => {
  const signers = [anaWallet];
  const { address: toAddress } = anaWallet;
  const value = ONE;

  gnosisSafe.setWallet(anaWallet);

  // Building actions won't be async anymore in the future
  const action = gnosisSafe.transferEther(signers, toAddress, value);

  const onConfirmation = message => {
    const { data } = message;
    const { args } = data;

    action.unsubscribe();

    // If we expose a blocking, sync version of getBalance as a helper we should be good
    // Alternatively convert getBalance to a promise can call done when it resolves?
    const balance = provider.getBalance(GNOSIS_SAFE_ADDRESS);
    expect(`${balance}`).to.equal(`${ZERO}`);
    done();
  };

  const onError = message => {
    const { error } = message;
    console.log(error);
    done(error);
  };

  action.send();

});

[pcowgill]

Or as a third alternative, call a function that's async from within the onConfirmation handler and keep the await approach for getBalance. Not sure if that makes sense though, I'd have to try it

[pcowgill]

Related: https://github.com/tasitlabs/TasitSDK/issues/128

[marcelomorgado]

Some comments:

• Building action is sync already. That specific test case don't because transferEthers() isn't a contract function, is a wrapper function that make some async calls to perform the transfer. I've noted that to discuss with you later on PR https://github.com/tasitlabs/TasitSDK/pull/217.

• Yes, we can expose a sync version of getBalance

• We can use https://www.chaijs.com/plugins/chai-as-promised/ on that example to the balance assertion.Something like that:

  // I need to find out how to convert `balance` to string
  return expect(balance).eventually.equal(`${ZERO}`);

• We are using await to mine blocks and do some assertion (pre/post conditions).

• I'm almost sure that we can have an async function (listener) assignment inside of a sync function (test case) (#128)

• Make sense to me not use waitForNonceToUpdate since we expect that our users will don't use that, but for the test case function maybe we can keep that as async because users can write react-native handler functions using async/await style, what do you think?

[pcowgill]

@marcelomorgado That all makes sense and sounds good!

[marcelomorgado]

@pcowgill commented:

I feel like we should be using try/catch with async/await almost always, and if I recall correctly there are some cases where we haven’t used it

[marcelomorgado]

I'm trying to keep async/await syntax and having event-driven test cases as we want to do on the app. Seems that mocha supports only one of these paths:

• async/await syntax (as we're doing now);
• sync function with done() callback; Isn't possible to combine both on the same test case, and it is desirable for us because of faucet/pre-conditions checks (that are mostly async). A way that we can have both is returning a Promise inside of the test case, it isn't syntax perfect but it's closest then the way that we're doing now.

Refs: https://github.com/mochajs/mocha/issues/2407#issuecomment-237414401 and https://github.com/mochajs/mocha/issues/2988#issuecomment-327200194 An example:

it("should buy an estate", async () => {
  const {
    assetId,
    nftAddress,
    seller,
    priceInWei,
    expiresAt,
  } = estateForSale;

  await checkAsset(estate, mana, estateForSale, ephemeralAddress);

  await expectExactTokenBalances(estate, [ephemeralAddress], [0]);

  const fingerprint = await estate.getFingerprint(`${assetId}`);

  marketplace.setWallet(ephemeralWallet);
  const executeOrderAction = marketplace.safeExecuteOrder(
    nftAddress,
    `${assetId}`,
    `${priceInWei}`,
    `${fingerprint}`
  );

  return new Promise((resolve, reject) => {
    const successfulListener = async message => {
      const { data } = message;
      const { args } = data;
      const { buyer } = args;

      marketplace.off("OrderSuccessful");

      expect(buyer).to.equal(ephemeralAddress);
      await expectExactTokenBalances(estate, [ephemeralAddress], [1]);

      resolve();
    };

    const errorListener = message => {
      const { error } = message;
      reject(error);
    };

    marketplace.on("OrderSuccessful", successfulListener);
    marketplace.on("error", errorListener);

    executeOrderAction.send();
  });
});

What do you think @pcowgill ?

[pcowgill]

@marcelomorgado Would another alternative be using a done() style for the test, but then having in internal unnamed async function wrapping pretty much everything in the test that lets you use awaits? Or is that disallowed?

[pcowgill]

In the current style where the test itself is async, we can't use await style for this line?

return new Promise((resolve, reject) => {

[pcowgill]

@marcelomorgado If you want to do it as written above, to perfectly follow the last example here - https://github.com/mochajs/mocha/issues/2988#issuecomment-327200194 - the executeOrderAction.send(); should move outside the promise, right?

[marcelomorgado]

@marcelomorgado Would another alternative be using a done() style for the test, but then having in internal unnamed async function wrapping pretty much everything in the test that lets you use awaits? Or is that disallowed?

Do you mean using an unnamed async function to the pre-conditions? If is it, I think I'll need to use the then function to run the safeExecuteOrder after the pre-conditions, right?

In the current style where the test itself is async, we can't use await style for this line? return new Promise((resolve, reject) => {

I'm thinking on that but for now I'm not sure how I can do the same as the resolve/reject functions. The advantage of the resolve function is that will wait until the listener be called (replacing the need of the waitForOneConfirmation).

@marcelomorgado If you want to do it as written above, to perfectly follow the last example here - mochajs/mocha#2988 (comment) - the executeOrderAction.send(); should move outside the promise, right?

Yes, actually the code above works if the action.send() be moved outside from the Promise.

I know that you wrote a draft for it before but I think this last one is a good example since we have 3 async calls before the call that we want to test, if you are seeing something that I'm not, a code sample could help me.

[pcowgill]

Do you mean using an unnamed async function to the pre-conditions?

No, I meant in the test itself. Or, what did you mean by pre-conditions here?

[pcowgill]

I'm thinking on that but for now I'm not sure how I can do the same as the resolve/reject functions.

I think async/await with a try catch where you return in either the try or the catch is the same as doing a resolve or a reject, right?

[pcowgill]

The advantage of the resolve function is that will wait until the listener be called (replacing the need of the waitForOneConfirmation).

Yep, we need to find a way to replace that. Just checking if resolve is the only way we can do it.

[pcowgill]

Yes, actually the code above works if the action.send() be moved outside from the Promise.

Cool. Let's move it then.

I know that you wrote a draft for it before but I think this last one is a good example since we have 3 async calls before the call that we want to test, if you are seeing something that I'm not, a code sample could help me.

Okay - go with this for now, and I'll play around with the options I was suggesting above

[marcelomorgado]

Do you mean using an unnamed async function to the pre-conditions?

No, I meant in the test itself. Or, what did you mean by pre-conditions here?

We have these async functions to check if the pre-conditions of the test are satisfied:

 await checkAsset(estate, mana, estateForSale, ephemeralAddress);
 await expectExactTokenBalances(estate, [ephemeralAddress], [0]);

And to call the safeExecuteOrder function we have to do that async call too:

  const fingerprint = await estate.getFingerprint(`${assetId}`);

[marcelomorgado]

I'm thinking on that but for now I'm not sure how I can do the same as the resolve/reject functions.

I think async/await with a try catch where you return in either the try or the catch is the same as doing a resolve or a reject, right?

Right, the difference is how to wait for something to happen. Seems that using async/await we have to use await something() and Promises uses something = () => { ...; resolve(); }

[pcowgill]

Right, the difference is how to wait for something to happen. Seems that using async/await we have to use await something() and Promises uses something = () => { ...; resolve(); }

Yep. So since async/await uses promises under the hood, why not use async/await instead of a promise?

[pcowgill]

We have these async functions to check if the pre-conditions of the test are satisfied And to call the safeExecuteOrder function we have to do that async call too

Ah ok, I see what you mean by pre-conditions now. Thanks.

Do you mean using an unnamed async function to the pre-conditions?

Yep, that's what I mean if we used the done() test style.

I think I'll need to use the then function to run the safeExecuteOrder after the pre-conditions, right?

Hmm, I don't think so. Couldn't this sync function also be in the unnamed async function? Or is safeExecuteOrder not really sync, just not awaited here?

[marcelomorgado]

Wow, I think that I've got it!

it.only("should buy an estate", done => {
  (async () => {
    const {
      assetId,
      nftAddress,
      seller,
      priceInWei,
      expiresAt,
    } = estateForSale;

    await checkAsset(estate, mana, estateForSale, ephemeralAddress);

    await expectExactTokenBalances(estate, [ephemeralAddress], [0]);

    const fingerprint = await estate.getFingerprint(`${assetId}`);

    marketplace.setWallet(ephemeralWallet);
    const executeOrderAction = marketplace.safeExecuteOrder(
      nftAddress,
      `${assetId}`,
      `${priceInWei}`,
      `${fingerprint}`
    );

    const successfulListener = sinon.fake(async message => {
      const { data } = message;
      const { args } = data;
      const { buyer } = args;

      marketplace.off("OrderSuccessful");

      expect(buyer).to.equal(ephemeralAddress);
      await expectExactTokenBalances(estate, [ephemeralAddress], [1]);

      done();
    });

    const errorListener = sinon.fake(message => {
      const { error } = message;
      done(error);
    });

    marketplace.on("OrderSuccessful", successfulListener);
    marketplace.on("error", errorListener);

    executeOrderAction.send();
  })();
});

[pcowgill]

@marcelomorgado Nice!!! Yeah that's just what we want.

[marcelomorgado]

This change should be extended to all test cases of the project?

[pcowgill]

This change should be extended to all test cases of the project?

I think so, yes. As long as you agree!

[marcelomorgado]

Ok, I agree, because of that I haven't marked this issue to be closed by the PR.

[pcowgill]

Ok, I agree, because of that I haven't marked this issue to be closed by the PR.

Great, thanks

[marcelomorgado]

I've removed the important and urgent label since the Decentraland.test.js test suite is using events style already.

[pcowgill]

I've removed the important and urgent label since the Decentraland.test.js test suite is using events style already.

Good call. Thanks.