referee.add(..) not working in setUp

[meisl]

This popped up in now-closed pr 2: consider

var referee = require("referee-combinators");
buster.testCase("abc", {
    setUp: function () {
        referee.add('customAssertion', ...);
    },
    tearDown: ...,
    "test customAssertion": function () {
        // something involving `combinators.assert.customAssertion(...)`
    }
});

...where referee.add is assumed to automatically create a suitable combinators.assert.customAssertion, as we already have it.

Problem is that this will yield a ReferenceError, "customAssertion" is not found on combinators.assert.

However, moving the referee.add outside buster.testCase makes it work:

var referee = require("referee-combinators");
referee.add('customAssertion', ...);
buster.testCase("abc", {
    "test customAssertion": function () {
        // something involving `combinators.assert.customAssertion(...)`
    }
});

Why is that?

[meisl]

To summarize what we have on the subject so far:

@johlrogge said:

[...] Something with this? :) Wouldn't that redefine the assert before each test is run but not really make a difference in what is left behind without a tear down?

@meisl replied:

[...] yes, must be some implicit object that's different. Could it be somehow different instances of referee? I mean of course the referee we see is the same in both scopes but some other copy of it being created behind the scenes? Hope @cjohansen will come to rescue again :)

Wouldn't that redefine the assert before each test is run but not really make a difference in what is left behind without a tear down?

Yes it would. Appropriate tearDown is to be added of course, but the fact that it's missing right now should not keep that test from passing, right?

@cjohansen came in (not yet to rescue, unfortunately):

[...] I'm a little stumped as to why you're seeing different behavior based on where the custom assertion is added. Seems like there are two instances of referee, and that shouldn't work any of the ways :S

Additionally there's another suspicion, takes a bit to explain: The "real bug" that original pr 2 was about turned out to be due to our using buster 0.6.12 from npm^[1] for executing the tests but acting on/extending referee in the code under test, plus a substantial difference between the old buster-assertions and the new referee.

Basically, it's this: you'll be sure to have a hard time asserting on assertions, beware of the meta :) ...and you can easily make times even harder by having meta- and object-level not fit very well...

Ok, so all above is to say: with said referee.add problem at hand, are we possibly again stepping into that same trap?

---

[1]: buster 0.6.12 doesn't know of referee-combinators, not even of referee (buster-assertions is now called referee)

[cjohansen]

Ah, I now see a possible solution. In order to add a custom assertion for using in your tests, you want to add it to Buster's instance (this will probably be true when Buster uses referee as well):

buster.assertions.add("custom", {});

[meisl]

Oh... I hadn't yet time to thoroughly check it. But still, even if that turns out to be working - I'm afraid I won't be really understanding how buster and referee/buster-assertions work together.

Could you give us a "101" on that? At best including how to get at the entrails, you know buster.assertions.equals vs buster.assert.equals - and that when given only referee (~> https://github.com/busterjs/referee-combinators/issues/5 "Don't rely on AssertionError from failure"). That would be really great! :)

[cjohansen]

buster depends on buster-assertions. In the main buster module, it exposes this dependency by doing something like

buster.assertions = require("buster-assertions");
// ...
module.exports = buster;

So, that means that buster.assertions in your tests comes from node_modules/buster/node_modules/buster-assertions/lib/buster-assertions.js. Your referee instance is a dependency in the project itself, so referee refers to the object defined in node_modules/referee/lib/referee.js.

In the next version of Buster, this problem will still exist, even if the buster-assertions project goes away. Starting from Buster 0.7, buster.assertions will be the object defined in node_modules/buster/node_modules/referee/lib/referee.js. So even if you now do require("referee") you will still get another object. This is simply how node packages work. In order to modify "Buster's referee instance", you will have to go through buster.assertions (buster.referee will also be exposed in 0.7).

Makes sense?

[meisl]

Wow man, you're fast - I am not :D Thx!

It's the best explanation so far of what was the problem in pr https://github.com/busterjs/referee-combinators/pull/2#issuecomment-13400881 (equals not doing coercion anymore in referee). Really, I didn't have it as clear as I have it now. So it's really the object from referee / buster-assertions. Is buster adding something to it or not? If so, when is this happening, ie every setUp/tearDown or per testCase or only on requiring buster?

However, it does not explain (I think) why referee.add(..) in setUp is not working while outside it is. Note, we're talking only referee, the very same thing in both cases. In fact, given your explanation, I am now wondering why it is working at all (in the latter case)... :(

Then there's this other question that I have, and I didn't pose it very well. I know now, as I discovered that I understand even less than I thought... So I'll try again; consider:

var assertionDef = {
    assert: function(actual, expected) {
        // ...
        return booleanResult;
    }
};
referee.add("foo", assertionDef);
// OR
buster.assertions.add("foo", assertionDef);

How can I call directly the function that's sitting at assertionDef.assert without having assertionDef in scope, ie. given only referee / buster.assertions? Or is it that I just can't? I do NOT want to call assert.foo, I want "the real thing". That's because I don't want failure events / AssertionErrors from it, rather I'd like to emit my own (and only one).

Another q, not so important: if I do add custom assertions in setUp, is there a (best) way to remove them in tearDown? Or is buster doing such rigorous housekeeping that it's simply unnecessary?

Lot's of questions... but you know, trying to learn :)

[cjohansen]

However, it does not explain (I think) why referee.add(..) in setUp is not working while outside it is. Note, we're talking only referee, the very same thing in both cases. In fact, given your explanation, I am now wondering why it is working at all (in the latter case)... :(

Like I said, this had me stumped too. I shouldn't have worked...

How can I call directly the function that's sitting at assertionDef.assert without having assertionDef in scope, ie. given only referee / buster.assertions? Or is it that I just can't? I do NOT want to call assert.foo, I want "the real thing". That's because I don't want failure events / AssertionErrors from it, rather I'd like to emit my own (and only one).

Interesting. Right now, you can't. But it makes all the sense in the world for referee to expose just the test function. Maybe something like this?

referee.add("something", {
    assert: function () { return true; }
});

referee.assert.something(); // Passes
referee.refute.something(); // Fails
referee.something(); // Returns true
// or maybe?
referee.fn.something();
referee.test.something();

Which one do you like? Other suggestions?

Another q, not so important: if I do add custom assertions in setUp, is there a (best) way to remove them in tearDown? Or is buster doing such rigorous housekeeping that it's simply unnecessary?

It's better to just add the assertion once globally. However, if you need to access instance-level state, then you can add it in setup and just forget about it. The next call will overwrite the previous one anyway.

[johlrogge]

My suggestion is to go all out and be able to get the original declaration. Perhaps completed with defaults for optional values. I'm thinking that a "combined" assert will want to emit one result but gather all "subresults" and therefore need both the function and to be able to build a message on failure.

Another option that I just thought of and like better is that assert.fn.something() could return a result that includes the message in case of failure rather than just true or false? Such as {status:'failure', message:'blargh'}. If success was undefined then one can just guard on var res = assert.fn.something(); if(res) {emit(constructCleverFailure(res)}

[meisl]

@cjohansen:

In fact, given your explanation, I am now wondering why it is working at all (in the latter case)... :(

Like I said, this had me stumped too. I shouldn't have worked...

Sorry, of course you did. Got confused of what who was thinking when (changes sometimes on my part...)

@cjohansen:

It's better to just add the assertion once globally. However, if you need to access instance-level state, then you can add it in setup and just forget about it. The next call will overwrite the previous one anyway.

Ok I see, no harm (side-effects) with adding the same thing again and again. But still, there's one use case that we might find ourselves in. Something like "if I do .add('foo', ...) then such-and-such appears elsewhere" plus "assuming I did NOT .add('foo', ...) then such-and-such (errors) will occur". Not sure if we will really need such tests but it's possible, we're in meta land...

So to be clear: currently there is no way to really remove a custom assertion once it's been added, right? Or did you mean by "instance-level state" that in a different context (other "sub-testCase", other "testCase") it simply won't be there? (and hence no need for a .remove)

Remark: overwriting with a bogus add('foo', { assert: function () { throw new Error(...); } }); doesn't count. Btw: what about the .expection in the assertion declaration? Would overwriting with a declaration without .expection actually remove a previously associated expectation? From what I (think I) understand of referee/expect.js I conclude the answer is NO (but didn't try) - hope I'm wrong on that...!

@cjohansen:

Which one do you like? Other suggestions?

@johlrogge:

My suggestion is to go all out and be able to get the original declaration.

Hehe, that's really interesting: I had read yours, @cjohansen before you had posted yours, @johlrogge. At that time, I tended to something pretty much like what you said afterwards, @johlrogge :) However, actually reading it made me think. I guess it was the "go all out" - I went "hmm, don't wanna lose my pants...". So what I mean by this: there's two different views

• ours: extending referee, poking at internals
• the user's, who writes tests using our stuff, be it new or old

Taking our view - putting aside the user's - I'm totally with you @johlrogge. Making good failure messages will be non-trivial, that's also a good point. I even think it will become really hard because we're going to layer assertions, and not only one level but any number (!). Hence there'll have to be some recursive data structure to pass on failure info. How to organize this in a robust and modular way? We'll see...

Now the user's view: buster is very keen on housekeeping, as it seems to me - and that's a good thing!^(TM) "Make it really hard to screw up internals - screwed up tests (user level) are hard enough". Something along these lines probably lead to what we're discussing here - why not just expose all internals of assertions (aka "go all out")? However, I do not know where to draw the line neither do I know how to draw it :) Just to point out.

So which one do I like? Sorry, don't have an answer yet.

[meisl]

Right now I'm stuck^[1] in a situation where I need user-level assertions like referee.assert.isX to have a pointer to their negation, s.t. referee.assert.isX.not === referee.refute.isX and vice versa.

Use case: (function f(a) {...})(referee.[assert|refute].isX) - within f I don't know whether it's refute or assert nor do I know that it's isX. Needed when building assertions from assertions (aka "combining"). Currently there are only .message (eg "Expected ${0} to be X") and possibly .expectationName (eg "toBeX").

Any objections against having such a .not on all of 'em?

^[1] looks like / I think - which, as said, can change...

---

One objection might be that it'd possible to write assert.isX.not(actual) in a test rather than refute.isX(actual). There exist alternatives though, maybe less harmful/dangerous (?):

• not(), a function returning the negation ~> "who the hell would write assert.isX.not()(actual)?" The not would throw (up) when fed any args ... hey, then why not even more ~> "sure anyone gives up before not()()()()!" 8-}
• .isAssert (boolean) plus .assertionName ("isX") s.t. in f: var neg = referee[a.isAssert ? "refute" : "assert"][a.assertionName];
• .assertionDef=== the object passed to referee.add PLUS .isAssert (or equiv).
• ...

[meisl]

Sorry, got yet another one - but now undoubtedly related to this issue:

"Who's your referee?"

... on user-level assertion referee.assert.isX or the assertion definition (=what is passed to referee.add), or both. So simply referee.assert.isX.referee === referee (and/or similar for the assertion definition). This - as opposed to .not above - should NOT be prone to abuse I think.

[meisl]

Errm, really apologizing for the flooding :}

Where to draw the line: could it be as simple as just between require("buster") and require("referee")?

The normal user would only do var B = require("buster") AND B.assertions / B.referee would only be a subset of what you'd get from require(referee). So with the latter we could really "go all out", exposing .assertionDef, .assertionName, .not, .referee , ... what have you? But buster.[assert|refute].isX or buster.assertions would expose only as much as now, maybe even less.

[johlrogge]

Whoaaa.... that was a lot o fstuff :) I'll poke out some nuggets and that caught my eye:

pants on; pants off

The second "all out approach" is the one I prefer best especially when considering mixing refutes and asserts in a complex structure. The it is better that when an assert or refute fails it takes care of bacing it's message an returning it in a result structure. That will tell parent assertions whether the child assertion failed or passed and what to output as a message when it failed. I'm thinking that composite failures could look a bit like json without the quotes and messages from child asserts as values. (and properly indented of course)

It will be hard wrap ones head around what it actually means to mix refutes and asserts like a madman. Not for a computer but for a human. My suggestion there is to recommend a less liberal mixing of refutes and asserts.

In this case buster would have two different wrappers (the same wrapper as today, and a wrapper for internals. The difference would be that the internal one would never emit and never throw, it would put together a result and return it (Actual) -> Result vs (Actual) -> Unit. (Actual) -> Result is combinable, (Actual) -> Unit is not.

.not or not .not

For composite assertions I don't see the problem if the result is managed as above. For describing asserts textually when asserting assertions my understanding is that all assertions under refute are wrapped in a function. They return a different message an should be described differently.

If not it could work that way without breaking any contracts with the user and the not that creates the refutewrapper does not have to be exposed to a user.

expose or not expose

What do we want to prevent?

1. protect the API from malicious users
2. not expose details for users to depend on before we are sure about them
3. protect users from making mistakes that will result in unexpected buster behaviors
4. keep the API simple in order not to overwhelm an end user

I'm thinking definitely not 1.

Probably 2.

I would not worry about 3, it is pretty unlikely that a user that can read would use the wrong thing by mistake, see 4.

4 would mean to store the internals under some object that is kept out of the users way but not hidden (or can perhaps be reached via a visitor).

I will need to get back with more thoughts but this is what I have so far in this time box :)

[cjohansen]

1. protect the API from malicious users

"Malicious users will malic" or something... :) Let's not worry about that.

1. not expose details for users to depend on before we are sure about them

That would be good, but on the other hand - we can control this via documentation as we have other places. If it ain't documented as part of the API, you depend on it on your own risk.

1. protect users from making mistakes that will result in unexpected buster behaviors

So long as the API is comprehensible, I don't see this happening too much.

1. keep the API simple in order not to overwhelm an end user

Yes, possibly.

If we can solve this without exposing assert.isString.not() I'd be more happy than if we introduce the not :)

[meisl]

If we can solve this without exposing assert.isString.not() I'd be more happy than if we introduce the not :)

So that's a not (or even a not.not.not?) for exposing .not at end user level (eg on assert.isX). Right? ~> +2 (= +1+1 ;) from my side, as I am, it seems, slightly more anxious than both of you, wrt all 4 points.

Anyways, I think we already know how to do it. As I said, it's NOT whether to .not or not to .not. Rather it's about where to draw the line. There are two degrees of freedom:

• buster.assertions / buster.referee need not expose everything exposed by referee
• since #10, at the latest, we're talking 3 stages/forms of assertions:
  1. lowest: the thing passed to .add, { assert: function () { ... } } (call it "the definition", maybe?)
  2. middle: what @johlrogge in #10 currently exposes as referee.assert.isX.interal (or .raw?), a "processed" definition
  3. top: referee.assert.isX

The two are completely independent^[1]. That is: whatever we decide to expose at 1, 2 or 3 we need not have exposed by buster.assert.isX.

Concretely: type (assert|refute), displayName and referee, as read-only props are OK on buster.[assert|refute].isX. No more and NO functions, that's important. These are also a minimal base I think. I'll argue for this alleged minimal set in more detail once I've tried a few more things.

On the other hand we're pretty free re what to expose at 1, 2 or 3 - and change our mind anytime with hardly any risk.

^[1]: actually that's a bit stupid to say, as it's implied by the term "degress of freedom"

[meisl]

...and:

In the style of Dr Seuss, considering the lots of nots and how we're used to use +1 - let's just say "nods" :)

[meisl]

Don't know if it's worth it but we could drop string property .type and instead use constructors Assertion and Refutation, making for even somewhat "less exposure".

Note that there's .assertMode on expectations, so that's what we could say instead of the rather unspecific "type" (in general, not only for the name of a property that we could do without anyways)