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

Output message when t.throws() fails #1408

Merged
merged 3 commits into from
Jun 13, 2017

Conversation

timdeschryver
Copy link
Contributor

Fixes #1342

Maybe the test can be written better?

lib/assert.js Outdated
pending(this, intermediate);
// Don't reject the returned promise, even if the assertion fails.
return intermediate.catch(noop);
return intermediate;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to remove the catch because otherwise I couldn't access the AssertionError in the test.

Copy link
Member

Choose a reason for hiding this comment

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

The catch is necessary. We don't expose the AssertionError instance to the test author. Instead add a .catch() callback in the pending() implementation here: https://github.com/tdeschryver/ava/blob/4a19a96da89a1533025295a7cd78b74c12528f04/test/assert.js#L15.

@timdeschryver timdeschryver force-pushed the message_throws branch 2 times, most recently from 050125c to 4a19a96 Compare June 10, 2017 11:05
Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

Looks good! The test does need to be approached differently. The promise returned by t.throws() should never reject.

lib/assert.js Outdated
let promise;
if (isPromise(fn)) {
promise = fn;
} else if (isObservable(fn)) {
promise = observableToPromise(fn);
} else if (typeof fn !== 'function') {
fail(this, new AssertionError({
assertion: 'throws',
assertion,
Copy link
Member

Choose a reason for hiding this comment

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

I think it's clearer to just repeat this property.

lib/assert.js Outdated
pending(this, intermediate);
// Don't reject the returned promise, even if the assertion fails.
return intermediate.catch(noop);
return intermediate;
Copy link
Member

Choose a reason for hiding this comment

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

The catch is necessary. We don't expose the AssertionError instance to the test author. Instead add a .catch() callback in the pending() implementation here: https://github.com/tdeschryver/ava/blob/4a19a96da89a1533025295a7cd78b74c12528f04/test/assert.js#L15.

test/assert.js Outdated
test('promise .throws() fails when promise is resolved', t => {
return assertions.throws(Promise.resolve('foo'))
.catch(err => {
testFailResults(t, err, {
Copy link
Member

Choose a reason for hiding this comment

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

Rather than extracting the comparison logic into testFailResults, if you capture lastFailure through the pending() function you could write an eventuallyFailsWith():

function eventuallyFailsWith(t, promise, subset) {
  return promise.then(() => failsWith(t, () => {}, subset))
}

Given that assertion.throws() should return a promise that always resolves, and the error will be captured separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip and reviews!
I got it to work now :), but I had to pass the lastFailure to the failsWith function otherwise it would be assigned to null. Is this OK?

Copy link
Member

Choose a reason for hiding this comment

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

My bad, overlooked that failsWith() resets lastFailure.

Instead extract the failure testing code into its own function, e.g.:

function assertFailure() {
  if (!lastFailure) {
  		t.fail('Expected assertion to fail');
 		return;
 	}
 
 	t.is(lastFailure.assertion, subset.assertion);
 	t.is(lastFailure.message, subset.message);
 	t.is(lastFailure.name, 'AssertionError');
 	t.is(lastFailure.operator, subset.operator);
 	if (subset.statements) {
 		t.is(lastFailure.statements.length, subset.statements.length);
 		lastFailure.statements.forEach((s, i) => {
 			t.is(s[0], subset.statements[i][0]);
 			t.match(s[1], subset.statements[i][1]);
 		});
 	} else {
 		t.same(lastFailure.statements, []);
 	}
 	if (subset.values) {
 		t.is(lastFailure.values.length, subset.values.length);
 		lastFailure.values.forEach((s, i) => {
 			t.is(s.label, subset.values[i].label);
 			t.match(s.formatted, subset.values[i].formatted);
 		});
 	} else {
 		t.same(lastFailure.values, []);
  	}
  }
}

And call that from failsWith() and eventuallyFailsWith() (so the latter doesn't call failsWith() anymore). eventuallyFailsWith() should also reset lastFailure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's indeed a cleaner solution.
Changes are commited!

@sindresorhus sindresorhus changed the title Message for t.thows() Output message when t.throws() fails Jun 13, 2017
@sindresorhus sindresorhus merged commit dfca2d9 into avajs:master Jun 13, 2017
@sindresorhus
Copy link
Member

Thank you @tdeschryver :)

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.

3 participants