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

Allow custom assert modules #49

Closed
wants to merge 5 commits into from
Closed

Allow custom assert modules #49

wants to merge 5 commits into from

Conversation

jenslind
Copy link

Allows you to set a custom assert module so it can be used together with plan().

var test = require('ava')
var assert = require('assert');
test.setAssertModule(assert);

#25

Jens Lind added 2 commits September 10, 2015 20:44
@Qix-
Copy link
Contributor

Qix- commented Sep 11, 2015

:O Yay. I don't know enough about the code base to see whether or not this is correct; @vdemedes and/or @sindresorhus will look it over, but this looks exciting.

@vadimdemedes
Copy link
Contributor

@jenslind First of all, thanks for PR! How would you implement a support for should interface?

a.should.equal(b);
should.not.exist(c);


// Set custom assert module
Runner.prototype.setAssertModule = function (assertModule) {
Test._setAssertModule(assertModule);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have 2 functions for a one task? You can just leave it as setAssertModule.

Copy link
Author

Choose a reason for hiding this comment

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

You mean like this?

Runner.prototype.setAssertModule = Test.setAssertModule;

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, I missed that there are Runner and Test there, sorry. I would leave setAssertModule on Runner and when you create a new instance of Test, assign it there - https://github.com/sindresorhus/ava/blob/master/lib/runner.js#L34.

@Qix-
Copy link
Contributor

Qix- commented Sep 11, 2015

should in and of itself doesn't have .plan(). It's on their radar, but it doesn't as of now (unfortunately). The question to be asked is what to do when .plan() is called. Doing nothing isn't ideal.

@sindresorhus
Copy link
Member

@Qix- Most assert libs doesn't have a .plan(). That's usually part of the test runner. What asked here is how to mixin should methods, since they're chained on the native prototype of objects/string/etc. The native assert module works here, since it's only methods on an object. But it doesn't work for more advanced cases, like should. Maybe we should also have an adapter interface of some kind that can handle more advanced cases. How would that look like though?

@Qix-
Copy link
Contributor

Qix- commented Sep 11, 2015

should is actually just the Should constructor returned by require('should'), and a binding of it is added to Object.prototype. Doing whatever wrapping you have for Should.prototype should work, and propagate to the Object.prototype invocations, if that's what you mean.

@sindresorhus
Copy link
Member

@Qix- Oh, I didn't realize. Yeah, that makes things easier. But there still might be other assertion libs where the easy way doesn't work. Maybe we should add support for the most popular assertion lib, so that you can just pass in the main export, and we'll handle whatever's nessecary to decorate it.

@jenslind
Copy link
Author

Updated the PR. The following should now work:

test('chai', function (t) {
  t.plan(2);
  t.assert.equal(true, true);
  t.expect(true).to.equal(true);
})

test('should', function (t) {
  t.plan(1);
  t.should(true).equal(true);
})

test('assert', function (t) {
  t.plan(1);
  t.equal(true, true);
})

Implementing support for a.should.equal(b) will need some more work, not sure how to achieve it. 🐈

});
}

function setPrototype(el, method) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Functions inside functions is a really no-go, please separate it out.

@vadimdemedes
Copy link
Contributor

@jenslind In my opinion, t.should, t.expect, t.assert is really not a desired solution for supporting custom assert modules. It shouldn't assign those modules to a test context. All it has to do is to patch them with an assert count for t.plan() to work.

@sindresorhus what do you think?

@jenslind
Copy link
Author

@vdemedes hmm okey so how would you patch the modules? Should we rather aim to add a counter to the module itself and then let t.plan() get the count from the module? But how would that work? A counter on the module won't know about which test it's in.

@Qix-
Copy link
Contributor

Qix- commented Sep 14, 2015

Remember - not all modules report successful asserts, which is something required for t.plan() to work. For instance, this was the problem with should - it doesn't, by default, report successful asserts.

@sindresorhus
Copy link
Member

The intention was to decorate the assert methods with an assertion counter, not mix them into the test context. I guess we could expose a t.countAssert() (better name?) or something too.

@vadimdemedes
Copy link
Contributor

I'd implement something like the following.

To use should:

var should = chai.should();
var ava = require('ava');

ava.should(should);

To use expect:

var expect = require('chai').expect;
var ava = require('ava');

ava.expect(expect);

or any other module, which does not require custom magic like should or expect does:

var assert = require('assert');
var ava = require('ava');

ava.assert(assert);

To avoid duplication, we could prefix those methods with setup:

ava.setupShould(should);
ava.setupExpect(expect);
ava.setupAssert(assert);

@Qix-
Copy link
Contributor

Qix- commented Sep 15, 2015

That seems messy for two reasons:

  1. should(should) is redundant. What else would you pass to should()?
  2. If you take that out, then you have to somehow enforce should be installed before calling ava.should(), unless you package all of them together, which... don't. :)

@sindresorhus
Copy link
Member

should(should) is redundant. What else would you pass to should()?

It's to be explicit. If we only have ava.setupAssert, we would have to detect should. Is that possible? If yes, how? Having only one setup method would indeed be nicer.

If you take that out, then you have to somehow enforce should be installed before calling ava.should(), unless you package all of them together, which... don't. :)

Not sure I understand what you mean, but this is only for mixing in the assert counter into the assert lib, not the other way. After calling ava.setupAssert(should), you would just call should normally, not through AVA.

I want to get this moving. What's the minium thing we can do first? Maybe ignore the should/except stuff and just expose a generic ava.countAssert() and ava.setupAssert().

@sindresorhus
Copy link
Member

Sorry @jenslind. Seems we aren't going to be able to reach a good solution for this right now. Closing for now, but happy to open it again when we have a decision on how to handle custom assertion libs.

Let's continue the discussion in #25.

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.

4 participants