test: add assertAmountsEqual() helper #2453

Chris-Hibbert · 2021-02-17T01:09:12Z

closes: #2442

katelynsills

Hmm, I think this effort might have to wait until after the amountMath refactoring. Here's why:

The function as written will pass even if the "amounts" aren't equal! See the zcf.makeInvitation - no customProperties test in test-zcf.js for an example of something passing when it shouldn't be. Even if we added more checks (such as .brand !== undefined) I think that would be the wrong solution. I think we actually want to use amountMath.isEqual to get the real, robust, and canonical answer on whether the two are equal. That will save us from any errors in trying to replicate it. (Of course, if we want to add other checks after it fails to provide a better error message we can. My worry is that we will have passes when we should have failures.)

But now, that means that we have to pass in an amountMath, which is difficult and annoying. I tried doing this, and I don't recommend it. However, after the amountMath refactoring, we won't need to.

So, I think this PR is the best that we could do now, but I think it's risky and I would probably prefer to redo it after the amountMath refactoring.

katelynsills · 2021-02-17T02:18:49Z

packages/zoe/test/unitTests/zcf/test-zcf.js

@@ -302,7 +303,7 @@ test(`zcf.makeInvitation - no customProperties`, async t => {
  // https://github.com/Agoric/agoric-sdk/issues/1705
  // @ts-ignore
  const details = await E(zoe).getInvitationDetails(invitationP);
-  t.deepEqual(details, {
+  assertAmountsEqual(t, details, {


Oh no, this is passing but it shouldn't. This is a record, not an amount, so when assertAmountsEqual checks .brand and .value both are undefined, and undefined === undefined so this passes.

Also, even if these were amounts, we should ensure this fails because MathKind.SET wasn't passed in, so MathKind.NAT was used as the default, and that is wrong.

I reverted this incorrect conversion fo deepEqual() to assertAmountsEqual(). I'll convert the PR to draft until we have a solution for AmountMaths.

blocked on: #2311

katelynsills · 2021-03-09T01:36:48Z

Hey @Chris-Hibbert, now that the new amountMath is in, we can revisit this!

switched from deepEqual() to assertAmountsEqual() on a non-amount.

katelynsills

We should never have to pass a MathKind in to know whether two amounts are equal. We should change this API

katelynsills · 2021-03-29T20:59:31Z

packages/zoe/test/zoeTestHelpers.js

+  t,
+  amount,
+  expected,
+  mathKind = MathKind.NAT,


This should be unnecessary if we are just comparing whether two values are equal.

katelynsills · 2021-03-29T21:00:21Z

packages/zoe/test/zoeTestHelpers.js

+  switch (mathKind) {
+    case MathKind.NAT:
+      valuesEqual = amount.value === expected.value;
+      break;
+    case MathKind.STRING_SET:
+      valuesEqual = setMathHelpers.doIsEqual(amount.value, expected.value);
+      break;
+    case MathKind.SET:
+      valuesEqual = setMathHelpers.doIsEqual(amount.value, expected.value);
+      break;
+    default:
+      valuesEqual = false;
+  }
+


Why are we doing this? amountMath.isEqual(x, y) will do everything we need to do.

This gives more detailed error messages when there isn't a match. If that's not valuable, then we can switch all the tests to use t.is(amountMath.isEqual(amount, expected, message)).

also, amountMath.isEqual() throws when the brands don't match, which is less helpful in a test.

We can still have the detailed error messages without having to pass in the mathKind. Some options:

Use amountMath.isEqual( directly. If we know the brands are the same and amountMath.isEqual is false, then we know the values are different and can use the appropriate error message.

Use getMathKind to switch which helpers to use based on the value alone.

Use isSetValue and isNatValue to switch which helpers to use based on the value alone.

Both of these don't need the mathKind to be passed in, so we should definitely remove that.

also, amountMath.isEqual() throws when the brands don't match, which is less helpful in a test.

An easy fix for this is only use amountMath.isEqual if you've already checked that the brands match

done. Thanks.

katelynsills

Looks good, thanks

Chris-Hibbert self-assigned this Feb 17, 2021

Chris-Hibbert added enhancement New feature or request hygiene Tidying up around the house Small test labels Feb 17, 2021

Chris-Hibbert added this to the Beta Launch milestone Feb 17, 2021

Chris-Hibbert requested a review from katelynsills February 17, 2021 01:26

katelynsills suggested changes Feb 17, 2021

View reviewed changes

Chris-Hibbert marked this pull request as draft February 17, 2021 17:19

Chris-Hibbert force-pushed the amountsEqual-2442 branch from 8b63cac to 37d1a21 Compare March 29, 2021 19:50

rowgraus removed this from the Beta Initial Launch milestone Mar 29, 2021

Chris-Hibbert force-pushed the amountsEqual-2442 branch from 37d1a21 to ccf6560 Compare March 29, 2021 20:12

Chris-Hibbert marked this pull request as ready for review March 29, 2021 20:34

Chris-Hibbert added 3 commits March 29, 2021 13:36

test: add assertAmountsEqual() helper

b35a023

chore: revert one incorrect (but illuminating) switch of deepEqual

ffe32c6

switched from deepEqual() to assertAmountsEqual() on a non-amount.

chore: integrate with amountMath changes and improve tests

3b56933

Chris-Hibbert force-pushed the amountsEqual-2442 branch from ccf6560 to 3b56933 Compare March 29, 2021 20:36

Chris-Hibbert requested a review from katelynsills March 29, 2021 20:37

katelynsills suggested changes Mar 29, 2021

View reviewed changes

refactor: drop MathKind parameter to assertAmountsEqual()

e72898f

Chris-Hibbert requested a review from katelynsills March 29, 2021 22:31

katelynsills approved these changes Mar 29, 2021

View reviewed changes

Merge branch 'master' into amountsEqual-2442

3a473cf

Chris-Hibbert enabled auto-merge (squash) March 29, 2021 22:51

Chris-Hibbert merged commit 2610f7c into master Mar 29, 2021

Chris-Hibbert deleted the amountsEqual-2442 branch March 29, 2021 23:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add assertAmountsEqual() helper #2453

test: add assertAmountsEqual() helper #2453

Chris-Hibbert commented Feb 17, 2021

katelynsills left a comment

katelynsills Feb 17, 2021

katelynsills Feb 17, 2021

Chris-Hibbert Feb 17, 2021

katelynsills commented Mar 9, 2021

katelynsills left a comment

katelynsills Mar 29, 2021

Chris-Hibbert Mar 29, 2021

katelynsills Mar 29, 2021

Chris-Hibbert Mar 29, 2021 •

edited

Loading

Chris-Hibbert Mar 29, 2021 •

edited

Loading

katelynsills Mar 29, 2021 •

edited

Loading

katelynsills Mar 29, 2021

Chris-Hibbert Mar 29, 2021

katelynsills left a comment

test: add assertAmountsEqual() helper #2453

test: add assertAmountsEqual() helper #2453

Conversation

Chris-Hibbert commented Feb 17, 2021

katelynsills left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katelynsills commented Mar 9, 2021

katelynsills left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chris-Hibbert Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

Chris-Hibbert Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

katelynsills Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katelynsills left a comment

Choose a reason for hiding this comment

Chris-Hibbert Mar 29, 2021 •

edited

Loading

Chris-Hibbert Mar 29, 2021 •

edited

Loading

katelynsills Mar 29, 2021 •

edited

Loading