Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: initial stab at trying Ava test framework #1447

Closed
wants to merge 38 commits into from
Closed

Conversation

dtribble
Copy link
Member

re #1335

A spike to see how AVA test frameworks. See #1335 for discussion.

Only ERTP tests were completely converted, though many tests required no additional changes to pass.

katelynsills and others added 30 commits August 8, 2020 18:27
- `getInviteIssuer` to `getInvitationIssuer`
- `inviteDesc` to `description`
zcf.addEmptySeat() would return a zcfSeat that the contract could use
to hold allocations.

untested
drop keyword parameter in addEmptySeat()
extract makeZoeSeatAdminKit, use it for emptySeat, too.
rename zcf seat construction to use 'Kit'
refactor: add types to reveal the wallet changes needed for Zoe
improved type annotations
rename makeEmptySeat to makeOfferlessSeat
don't return the zcfSeatAdmin to the contract
feat: add the ability for a contract to get a synchronous seat
Based on the synchronous seat work.

The unit tests for autoswap pass. I haven't yet done the swingset tests.
refactor: bring autoswap up to date on the new Zoe spike branch
michaelfig and others added 7 commits August 10, 2020 17:00
chore: add hasExited, getNotifier to UserSeat
* chore: update tests

* chore: finish automaticRefund tests

* chore: update barterExchange and brokenContract tests. Small changes to Zoe and types (#1427)
fix: get wallet tests to pass with new Zoe API
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LATM --- Looks Awesome To Me. Very enthusiastic!

@erights
Copy link
Member

erights commented Aug 12, 2020

LATM --- Looks Awesome To Me. Very enthusiastic!

Just to be clear, this wasn't approval. It is happy enthusiasm.

Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like t.notThrows(). I think it may be the answer to the issuer I've been pointing to.

Comment on lines +174 to 184
return E(issuer)
.burn(payment1, amountMath.make(837))
.then(burntBalance => {
t.ok(
t.assert(
amountMath.isEqual(burntBalance, amountMath.make(837)),
`entire minted payment was burnt`,
);
t.rejects(() => issuer.getAmountOf(payment1), /payment not found/);
return t.throwsAsync(() => issuer.getAmountOf(payment1), { message: /payment not found/ });
})
.catch(e => t.assert(false, e));
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t.notThrows() would be good here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would our rule be to put t.throwsAsync or t.notThrows (t.notThrowsAsync??) around every call that returns a Promise? I can't imagine us being ok with any potential rejection going unnoticed, so every promise would fall into one of:

  • we expect a positive result and want to examine it: use checkResult(await ..) or t.notThrows(..).then(checkResult)
  • we expect a positive result but don't care about the value: use await .. or t.notThrows(..);
  • we expect a negative result (testing delibrary failure situations): use t.throwsAsync(..);

For sync stuff, I expect an exception that throws high enough to be noticed by the enclosing test case to flunk the test. As long as any lower-level exceptions cause a Promise to reject, and we don't drop any promises on the floor inside our libraries, and our test cases are on the lookout for rejections, then arbitrary exceptions in our libraries ought to make the test cases flunk too.

Base automatically changed from zoe-release-candidate to master August 17, 2020 04:13
@warner warner mentioned this pull request Aug 20, 2020
35 tasks
@michaelfig
Copy link
Member

@dtribble this PR looks like it's been done by @warner's latest changes. Closing, but feel free to reopen if you need it for reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants