rewrote tests, revealed (real) bug

[meisl]

I've pushed the tests a bit further in the direction started by @johlrogge, namely (more) similar to how assertions are tested in referee itself. I turned around a few things so the appearance is now a bit different.

Main goals were to have better test names, st. in case of failure (of referee-combinators tests) it's really clear and concise what has been tested, and also to make it more flexible st. new kinds of tests can be added more easily (both pretty much achieved, IMO). As an example for the latter: I found that it should be possible to give an optional failure message which should be passed on to the underlying "normal" assertion, and implemented it. What's missing is tests for it so that will show if I made the scheme sufficiently flexible.

Another thing (not yet done) is that we need to improve the way failure / passing (of constructed 'partial' assertions) is being tested for, simple [assert|refute].exception(..) ain't enough. Consider referee.throwOnFailure = false...

What I did so far actually revealed a real bug (as opposed to bug in tests), as I think: assert.equals(42, "42") passes while combinators.assert.equals(42)("42") fails [EDIT: this should rather be combinators.assert.equals("42")(42), see below].

Well, ideally the tests for combinators should verify this statement: "ALL 'partially applied' assertions from combinators behave exactly like the 'normal' (except for how they're called)". Something like "in appropriate manner, check all tests that exist for referee assertions on combinator assertions as well".

Beware: this pr is far from done, it is rather TBD.

[johlrogge]

The assert.equals(42, "42") would translate to combinators.assert.equals("42")(42) <-- actual last. My guess is that assert.equals("42", 42) also fails?

[meisl]

Sorry, mixed it up in description (but not in actual test).

Nevertheless, tried it the other way round (swap e and a in test "to be sure"): same result.

[johlrogge]

What I'm currently doing is look into making the "attribute" combinator Basically apply assertion z to the attribute y of actual x. I'll have a look at your changes to see what I can use. I have a working test but I want to refactor it a bit before I push.

[meisl]

refactor code (no problem) or refactor test?

[meisl]

I mean if you go by the latest test-style I've seen from you and don't refactor your testPartial too much I can just merge your stuff and adapt. The test style that I'm suggesting is still TBD, after all.

[johlrogge]

It's new tests, they don't have too much in common with the previous tests that just tests mechanics. The new tests should end up more in the style of referee since it's actually new types of asserts that's tested.

[johlrogge]

Very weird that you get different results with combinator.assert and referee.assert. Any ideas why that is?

[meisl]

It's new tests, they don't have too much in common with the previous tests

Ok, so we'll have to compare. Just let me know when you've pushed and I'll try to summarize the differences in approach here.

Very weird that you get different results with combinator.assert and referee.assert. Any ideas why that is?

Strange indeed! I think you can guess why I added a test called "to be sure"... Must be a very intricate thing, but so far the only suspicion I've got (NOT: "idea") is my usual first one: maybe something with this?

[meisl]

Just thought you might find it helpful to know my thinkings behind "turning around a few things":

• separate (creation of function for) testing failure and pass: we'll have to assert on the fail and emit events (and the fail message) rather than simply assert on AssertionException, so their implementation will considerably differ.
• separate "correct" and "incorrect" (were args to your testPartial helper fn): usually it's not symmetric, ie. we may want more tests with "correct" values than with "incorrect" ones, or vice versa.
• what is symmetric, however, is assert vs refute, so I chose to construct that automatically (as opposed to your separated sub-test-cases "assert" and "refute")
• make really clear what the derived assertion is applied to in the first step vs in the second. Variadic testPartial got me confused too many times, so
  • the first-step args ("expected" and optional failure message) are now specified as an array
  • ...and that's done pretty "far away" from the "second-step" arg ("actual"), using callback functions pass and fail like in tests for referee. Others (like msg in tests for referee) can be added.

Note: currently the "first-step" args are still passed to my makeTests but that's not essential. It might turn out better to also pass them to pass or fail (which, when called, finish defining the particular tests to be constructed).

[johlrogge]

It all looks much better than my initial tests. Didn't have time to do anything myself yesterday but hopefully tonight.

[cjohansen]

42 vs "42" - this is supposed to fail. The coercion was recently dropped. If you're seeing anything else I'd guess you have two different versions of referee going on?

[meisl]

Ooh... Thanks man, that is it. The problem is that buster 0.6.12 from npm still has the old buster-assertions with coercing equals but referee has the non-coercing equals. So assert.equals(act, exp) comes from buster 0.6.12 but combinators.assert.equals(exp)(act) from referee.

So should we rather use all-latest code (dev-env)? I'm not sure if I can get this reasonably running over here :( Windows, you know.

Btw, that's a pretty "big-ass" change. I'm wondering how many tests out there will break when 0.7 comes out...

[meisl]

^^^ Now only one test failing, "check equals coercing or not?". It says normal equals should NOT do coercion: 42 expected not to be equal to 42 btw, it should show the 2nd ("expected") in quotes.

[johlrogge]

How about deferring that test: "// ..." : function and then pull it into develop?

[meisl]

Got it working with the dev-env (and no failure).

I need to require buster-node instead of buster there (until it's ready for 0.7) but I also want buster.format.ascii. Here's a workaround:

        // This is a temporary workaround for the development-environment:
        buster = require("buster-node");
        var B = require("buster");
        buster.format = { ascii: B.format.ascii.bind(B) };
        // remove the above and uncomment the following once workaround is obsolete:
        //buster = require("buster");

But deferring is a better idea, we might find more discrepancies. And now we know what's the problem. So my last commit does exactly that.

[meisl]

It all looks much better than my initial tests. Didn't have time to do anything myself yesterday but hopefully tonight.

Thx :) Got some time now and thought I'd start improving testing for pass/failure. Or should I better wait to see what you were doing?

[meisl]

Refactored tests a bit and tried to move the referee.add({...}) (custom assertion) into setUp - won't be found; seems it must be outside buster.testCase(...). Why's that?

[johlrogge]

I pushed what I have now. It is an experiment of combining assertions. This technique won't work properly for or-combinators etc since we need to intercept emissions of failures and counting of subasserts. But it is a start. I think we will need to poke around a little in referee to allow what we want to do later. Not sure about the best strategy though but it doesn't prevent experimenting with some combinators anyway.

[meisl]

@johlrogge, I did the merge and conflicts were easy except one: how partial passes on additional args (eg optional failure message). See top of this commit for comparison.

I think it should be possible to give an optional failure not only when preparing as in

var assertGreaterThan1 = combinators.assert.greater(1, "more than 1");

but also when executing as in

[...].then(assertGreaterThan2(spy.callCount, "spy should have been called more than once"))
[...]
     .then(assertGreaterThan2(listeners.length, "foo should have more than one listener"))

(sorry, example is a bit contrived) But it's also more intricate than my previous solution, because it depends on the arity of the assertion which arg is which (and which are optional). Had I only written some tests...

Can you agree in principle, that it's a desirable use-case?

Left in yours for now and will try to consolidate the integrate branch a bit.

[johlrogge]

Hi Mathias,

TLDR version I agree it's a usecase but I propose a combinator solution (also your example seemed to use promises and then the actual would be passed as a resolved value. I changed my examples below accordingly) (perhaps optional messages should always be handled with combinators?).

long example (3 actually) of what I mean

I'm not married to any of the names, made them up quick here to write the example

Here is a meta codeish example:

when(spy).
then(assert.attr('callCount', withMessage(assertGreaterThan2, "spy should have been called more than once"))).
then(assert.attr('listeners', assert.attr('length', withMessage(assertGreatherThan2, "foo should have more than one listener"))));

I'm currently working on assert.structure which is essensially a sugar that allows the above to be written as:

when(spy).
then(assert.structure({
  'callCount': withMessage(assertGreaterThan2, "spy should have been called more than once")
})).
then(assert.structure({
   'listeners':{
      'length': withMessage(assertGreatherThan2, "foo should have more than one listener")
}));

The underlying combinators would look exactly the same. It could also be simplified as

when(spy).
then(
  assert.structure({
     'callCount': withMessage(assertGreaterThan2, "spy should have been called more than once"),
     'listeners':{
         'length': withMessage(assertGreatherThan2, "foo should have more than one listener")
     }
}));

[johlrogge]

One can also imagine chaining a withMessage to all cominators:

when(spy).
then(
  assert.structure({
     'callCount': assertGreaterThan2.withMessage("spy should have been called more than once"),
     'listeners':{
         'length': assertGreatherThan2.withMessage("foo should have more than one listener")
     }
}));

But I don't see how the message could be an optional argument in a promise chain. It could be supported outside of promises though which may be desired. I'm leaning towards keeping it simple at first since I fear optional arguments to the actual can bite us (I don't really feel I grasp all the ins and outs yet so I suggest we aim to explore the combinator part of it first, perhaps not supporting all the nittygritys or regular asserts up front)

[meisl]

Very cool, didn't think of that! You're right that optional args can bite us badly. I'd prefer the chained .withMessage as it reads more like proper English.

Then I'd like to report on my progress on integration. Actually it was surprisingly easy to write your tests for attr using my makeTests (not married to this name, btw). At the moment test names are for the most part constructed, lacking a custom description as you had it, eg "for equal and other attributes". Here's an example for comparison:

was: 
extended asserts one attribute fail for missing attribute

now:
extended asserts one attribute refute.attr("name", assert.equals(the name))({  }) should return actual value
extended asserts one attribute refute.attr("name", assert.equals(the name))({  }) should pass
extended asserts one attribute assert.attr("name", assert.equals(the name))({  }) should fail
extended asserts one attribute refute.attr("name", assert.equals(the name)) should have .displayName

Note that it's four tests that come from one line, fail({}); // pass for missing attribute. And of course with less stuff around it, as you've already seen before. I think an optional message arg for pass and fail callbacks is ok (only these, wrt to partial assert I totally agree with you). The last test gives a hint how I'm doing it.

This is only to give you a taste, I'm not done yet. Will try to merge back asap so you can pull, but it can take me a while.

[johlrogge]

Awesome. Your way is much better. I knew you were working on your assertions so I didn't put too much effort into duplicating your work. Just enough to be able to my stuff to work.

I'd rather pull something ok sooner than something perfect later. I can always pull improvements later.

About .withMessage I prefer that too. I think that the underlying implementation will probably look something like the underlying alternative though. I see it as sugar.

In general my strategy now is to focus on how to effectively combine assertions in a functional way. I have deferred the details of other very important things such as understandable error messages etc for later. I'm starting to reach a point now where I really need to reread that blog about parser combinators. I remember there were lots of very elegant solutions to problems I'm about to face soon :)

[meisl]

Phew... it's working, plz pull. Your latest stuff re structure is also in there so you should get no conflicts (just a fast-forward).

I've got to clean up quite a bit but as you said, rather sooner ok than perfect later.

[meisl]

Sorry, forgot to say that 4 tests fail, due to incomplete impl of .displayName, nothing serious (cosmetic).

[johlrogge]

Thanks! It's merged. I will play with it at home.

[meisl]

Wish you a lot of fun :)

I've got some of the clean-up done but not all. It's not essential but you might want to pull directly from my fork, perhaps for reducing risk of divergence. If you plan to do so, plz tell me. I would then refrain from force-pushing every once in a while and perform major surgical action in separate branches only.