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

marcelomorgado · 2019-02-25T18:17:25Z

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:
#207 (comment)
mochajs/mocha#2988 (comment)
https://mochajs.org/#asynchronous-code

The text was updated successfully, but these errors were encountered:

marcelomorgado · 2019-02-25T18:18:50Z

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

marcelomorgado · 2019-02-26T17:21:51Z

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 · 2019-02-26T17:45:30Z

@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 · 2019-02-26T17:47:23Z

@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 · 2019-02-26T19:08:02Z

@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 · 2019-02-26T19:54:37Z

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

pcowgill · 2019-02-26T19:57:50Z

@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 · 2019-02-26T20:00:06Z

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 · 2019-02-26T20:25:26Z

Related: #128

marcelomorgado · 2019-02-26T20:38:44Z

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 Gnosis Safe bootstrap #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) (Select whether or not we want to allow async listeners (handlers) for events or confirmations in TasitAction #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 · 2019-02-27T16:17:42Z

@marcelomorgado That all makes sense and sounds good!

marcelomorgado · 2019-02-27T20:06:07Z

@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 · 2019-04-30T14:21:23Z

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: mochajs/mocha#2407 (comment) and mochajs/mocha#2988 (comment)
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 · 2019-04-30T14:42:15Z

@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 · 2019-04-30T14:44:29Z

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 · 2019-04-30T14:48:30Z

@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?

marcelomorgado · 2019-04-30T15:05:56Z

@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 · 2019-04-30T17:42:25Z

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 · 2019-04-30T17:43:26Z

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 · 2019-04-30T17:44:14Z

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 · 2019-04-30T17:45:19Z

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 · 2019-04-30T17:46:51Z

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 · 2019-04-30T17:49:09Z

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 · 2019-04-30T18:40:11Z

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 · 2019-04-30T18:43:42Z

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 · 2019-04-30T18:55:09Z

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 · 2019-04-30T19:47:03Z

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

marcelomorgado · 2019-05-02T17:20:10Z

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

pcowgill · 2019-05-02T17:33:09Z

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

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

marcelomorgado · 2019-05-02T18:56:47Z

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

pcowgill · 2019-05-02T19:09:22Z

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

Great, thanks

marcelomorgado · 2019-05-08T10:34:24Z

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

pcowgill · 2019-05-08T12:16:49Z

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

Good call. Thanks.

marcelomorgado added this to the 0.2.0 release milestone Feb 25, 2019

marcelomorgado added the tech debt Technical debt not involving bugs or features label Feb 25, 2019

marcelomorgado mentioned this issue Feb 25, 2019

TasitSDK ropsten fork test suite #207

Merged

pcowgill changed the title ~~Test suites should captures how we expect the library to be used~~ Test suite should capture how we expect the library to be used Feb 26, 2019

marcelomorgado modified the milestones: 0.2.0 release, 0.1.0 release Apr 25, 2019

marcelomorgado added the important and urgent label Apr 25, 2019

marcelomorgado mentioned this issue Apr 26, 2019

Action.send(); #357

Merged

pcowgill mentioned this issue May 1, 2019

Using events and/or action confirmation listeners instead of waitForOneConfirmation tasitlabs/tasit-apps#151

Open

marcelomorgado mentioned this issue May 1, 2019

Test cases from waiting to event driven style #369

Merged

marcelomorgado mentioned this issue May 3, 2019

ESLint #379

Merged

marcelomorgado removed the important and urgent label May 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

marcelomorgado commented Feb 25, 2019

marcelomorgado commented Feb 25, 2019

marcelomorgado commented Feb 26, 2019

pcowgill commented Feb 26, 2019

pcowgill commented Feb 26, 2019

marcelomorgado commented Feb 26, 2019

pcowgill commented Feb 26, 2019

pcowgill commented Feb 26, 2019

pcowgill commented Feb 26, 2019

pcowgill commented Feb 26, 2019

marcelomorgado commented Feb 26, 2019

pcowgill commented Feb 27, 2019

marcelomorgado commented Feb 27, 2019

marcelomorgado commented Apr 30, 2019

pcowgill commented Apr 30, 2019 •

edited

Loading

pcowgill commented Apr 30, 2019

pcowgill commented Apr 30, 2019

marcelomorgado commented Apr 30, 2019

pcowgill commented Apr 30, 2019

pcowgill commented Apr 30, 2019

pcowgill commented Apr 30, 2019

pcowgill commented Apr 30, 2019

marcelomorgado commented Apr 30, 2019

marcelomorgado commented Apr 30, 2019

pcowgill commented Apr 30, 2019 •

edited

Loading

pcowgill commented Apr 30, 2019

marcelomorgado commented Apr 30, 2019 •

edited

Loading

pcowgill commented Apr 30, 2019

marcelomorgado commented May 2, 2019

pcowgill commented May 2, 2019

marcelomorgado commented May 2, 2019

pcowgill commented May 2, 2019

marcelomorgado commented May 8, 2019

pcowgill commented May 8, 2019

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

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

Comments

marcelomorgado commented Feb 25, 2019

marcelomorgado commented Feb 25, 2019

marcelomorgado commented Feb 26, 2019

pcowgill commented Feb 26, 2019

pcowgill commented Feb 26, 2019

marcelomorgado commented Feb 26, 2019

pcowgill commented Feb 26, 2019

pcowgill commented Feb 26, 2019

pcowgill commented Feb 26, 2019

pcowgill commented Feb 26, 2019

marcelomorgado commented Feb 26, 2019

pcowgill commented Feb 27, 2019

marcelomorgado commented Feb 27, 2019

marcelomorgado commented Apr 30, 2019

pcowgill commented Apr 30, 2019 • edited Loading

pcowgill commented Apr 30, 2019

pcowgill commented Apr 30, 2019

marcelomorgado commented Apr 30, 2019

pcowgill commented Apr 30, 2019

pcowgill commented Apr 30, 2019

pcowgill commented Apr 30, 2019

pcowgill commented Apr 30, 2019

marcelomorgado commented Apr 30, 2019

marcelomorgado commented Apr 30, 2019

pcowgill commented Apr 30, 2019 • edited Loading

pcowgill commented Apr 30, 2019

marcelomorgado commented Apr 30, 2019 • edited Loading

pcowgill commented Apr 30, 2019

marcelomorgado commented May 2, 2019

pcowgill commented May 2, 2019

marcelomorgado commented May 2, 2019

pcowgill commented May 2, 2019

marcelomorgado commented May 8, 2019

pcowgill commented May 8, 2019

pcowgill commented Apr 30, 2019 •

edited

Loading

pcowgill commented Apr 30, 2019 •

edited

Loading

marcelomorgado commented Apr 30, 2019 •

edited

Loading