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

chore: update 'SwingSet' package to use AVA for testing #1579

Merged
merged 6 commits into from
Aug 21, 2020

Conversation

warner
Copy link
Member

@warner warner commented Aug 21, 2020

refs #1568

@warner warner marked this pull request as ready for review August 21, 2020 06:55
@warner warner requested a review from dtribble August 21, 2020 06:55
Copy link
Member

@dtribble dtribble left a comment

Choose a reason for hiding this comment

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

There are a few tests that need plan, but they are not blocking.

isSide(objP, side) {
return E(objP)
.side()
.then(s => t.is(s, side, `obj.side() is ${side}`));
Copy link
Member

Choose a reason for hiding this comment

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

Given the await style, these actual assertions should be moved out to the outside. This is a case where this test woudl require a `plan`` because who knows whether the assertions will simply never get invoke and so not have a chance to fail. Bit if they are moved to the top instead, then it would just flow.

));
const rightRef = leftBootstrap();

await E(rightRef).isSide(rightRef, 'right');
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps

Suggested change
await E(rightRef).isSide(rightRef, 'right');
cons rside = await s.side();
t.is(rside, 'right');

test('try disconnecting captp', async t => {
const objs = [];
const rejected = [];
const { getBootstrap, abort } = makeCapTP(
Copy link
Member

Choose a reason for hiding this comment

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

all the makeCapTP invocations would be clearer if the fancy arguments were refactored into named consts.

import { E, makeCapTP } from '../lib/captp';

test('try disconnecting captp', async t => {
const objs = [];
Copy link
Member

Choose a reason for hiding this comment

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

I have no idea what this test is trying to do, but it really needs a plan(X) to make sure that assertions are gettign executed.

() => ns4.f7WriteGlobal(5),
'Cannot assign to read only property',
{ message: /Cannot assign to read only property/ },
Copy link
Member

Choose a reason for hiding this comment

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

Why switch from exact mach to RE within the error string?

Copy link
Member Author

Choose a reason for hiding this comment

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

the test failed when I did that, apparently the full error string is longer than that, and tap/tape's assertion was actually doing a substring match

@warner
Copy link
Member Author

warner commented Aug 21, 2020

thanks, I'll schedule those fixes for a subsequent PR

@warner warner merged commit 23da2ca into master Aug 21, 2020
@warner warner deleted the 1568-test-with-ava branch August 21, 2020 23:23
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.

2 participants