busterjs / referee-combinators

Combinators goes assertion
0 stars 1 forks source link

Asynchronous combinations and exceptions are not buddies #10

Open johlrogge opened 11 years ago

johlrogge commented 11 years ago

I have been debating failures as exceptions a bit with myself now and it basically boils down to this:

The price to support failures as exceptions is too high

With combined asserts it makes sense to be able to assert in structures where some steps may be asynchronous (either via promises or callbacks). If this happens referee can still support this via failures as events but not as exceptions. (since asynchronous results will arrive later and when the result arrives it is via a different callstack).

Why support exceptions

Other testframeworks may rely on them in order to use referee. To report failures as exceptions is common in xUnit frameworks but is very ill suited for the the threadingmodel in javascript (you can for instance not block in a thread while waiting for a callback since there really is only one thread except for IO that can be concurrent). Other slicing is done by delaying ticks and as soon as you tick you can't throw anything back to the originator.

For buster on the other hand we have control and can decide that buster uses the event model for assertions and doesn't use exceptions.

Branch off

I'm feeling that we are compromising on combinators too early. I would like to explore how it can work if we don't need to support exceptions. I thought to branch of with that constraint lifted and see what the unconstrained potential is. Possibly referee can still support exceptions while referee-combinators will require the eventmodel. Basically you can use what exists today but if other frameworks want to use referee-combinators they will have to bend our way (rather than the other way around).

I have already forked referee, my intention is to explore and show which changes I think are needed. I see no reason why I would have to change existing contracts for existing assertions but I will adapt the internals a bit (as little as possible) to fit my needs.

When I'm done (and while I'm doing it) we can debate pros and cons. I'm assuming @meisl will continue to explore the current path a bit longer?

johlrogge commented 11 years ago

This will mean that I'm also taking a stab at #5 but from a different angle and with less constraints

meisl commented 11 years ago

First off, I'm neither a decided fan of failure-by-exception nor am I completely against it, just taking it as given. I suspect there's a reason for AssertionError being there, something like making buster most open for other assertion libs or so. But in principle the events would suffice, I guess (@cjohansen ?)

Anyways, having both seems to be what's making things hard. So +1 for exploring a pure way.

Basically, what I need - and referee-combinators needs - urgently - is a clean and reliable way to assert on assertions. Well not really big news, is it? ;) Ideally we'd have something like assert.passes(assertion, args) and - more interesting - assert.fails(assertion, args, and-then-assert-on-the-failure-message-but-how?), no matter if assertion is from the same referee as passes & fails or not.

I do agree that exceptions seem to make this (or something like it) harder to achieve. Also, your suggestion (elsewhere) about making failure explicit, ie having an object representing the failure and its various aspects is GREAT! We must pursue this. What we have now, AssertionError with a string msg and/or stuff passed to failure event listener ain't enough and not very well organized either.

From work on ramp-resources [link is to come] I know a rather clumsy way to do such "meta-assertions", a combination of try-catch and intercepting buster's fail. Brittle enough... So that is what I was going to start with re #5. But it's far from ideal, just to have something. I won't have a problem with it being replaced by something more elegant and/or powerful, that's for sure :) It is, however, poking around at quite a deep level (I mean in general, not my particular approach or yours). Therefore it will be important to keep an eye on not diverging too much. You know, I don't even wanna imagine what kind of nasty bugs might arise otherwise...

I have already forked referee

So that means once you're out of experimental stage we will really need to get clear about the "buster 0.6.12 for doing tests vs which-referee-is-it?" problem. You know, many things have been tried already: drop a referee clone into node_modules / work with dev-env (~> buster itself will be from there) / ... Fun fact: it's actually our weird having-different-instances-of-referee that let us get along so far with just catching AssertionError for detecting failure.

So: yea, plz go ahead exploring! Looking forward to your findings :) Let's stay close together on how failure will be represented in an object (as you had proposed). Let's come up soon with a definition of the working environment (referee from where, buster from where?). I'll try to apply my approach from ramp-resources to get something, and do my best to make it exchangeable.

johlrogge commented 11 years ago

First off, I'm neither a decided fan of failure-by-exception nor am I completely against it, just taking it as given. I think we are pretty much on the same page here. I have just started to be against them as I think I'm beginning to understand what can't be achieved with them.

I suspect there's a reason for AssertionError being there, something like making buster most open for other assertion libs or so. I think it is becacause that is generally how assertions work as a legacy from xUnit frameworks which has lead to that other JS-frameworks use the same principle. So in order for referee to be useful from as many other frameworks as possible exceptions need to be supported. I guess

Anyways, having both seems to be what's making things hard. So +1 for exploring a pure way.

Actually no, having exceptions at all as a mechanism to report failures limits the assertions usefulness in an asynchronous scenario wether it is the only way or one of several ways.

Basically, what I need - and referee-combinators needs - urgently - is a clean and reliable way to assert on assertions. Well not really big news, is it? ;)

Actually I think what I'm currently working on will solve that. I will push something as soon as I have something bit in principle you can test every aspect of how the assertion works on the internal assertion (for lack of better terms). The internal assertion is used by referee to verify expected and actual. The internal assertion returns it's result (currently as a promise, more on that later) and the outcome can be verified outside of referee.

Then the testing problem breaks down into two parts:

  1. how does the internal assertion behave
  2. how does referee interact with any internal assertions

Ideally we'd have something like assert.passes(assertion, args) and - more interesting - assert.fails(assertion, args, and-then-assert-on-the-failure-message-but-how?), no matter if assertion is from the same referee as passes & fails or not.

I'll keep this in mind. Stay tuned, splitting out internal assertions changes everything

I do agree that exceptions seem to make this (or something like it) harder to achieve. Also, your suggestion (elsewhere) about making failure explicit, ie having an object representing the failure and its various aspects is GREAT! We must pursue this. What we have now, AssertionError with a string msg and/or stuff passed to failure event listener ain't enough and not very well organized either.

I think I have this covered too.

It is, however, poking around at quite a deep level (I mean in general, not my particular approach or yours). Therefore it will be important to keep an eye on not diverging too much. You know, I don't even wanna imagine what kind of nasty bugs might arise otherwise...

Yes, I will try to update as often as possible when I'm at a place where it doesn't just add to the confusion (as is the case now)

So that means once you're out of experimental stage we will really need to get clear about the "buster 0.6.12 for doing tests vs which-referee-is-it?" problem. You know, many things have been tried already: drop a referee clone into node_modules / work with dev-env (~> buster itself will be from there) / ...

Actually it is the first experiment and what I feel is the most promising. I simply got tired of tip toing around referee and catering for these constraints was more than my poor little brain could handle.

Fun fact: it's actually our weird having-different-instances-of-referee that let us get along so far with just catching AssertionError for detecting failure.

So: yea, plz go ahead exploring! Looking forward to your findings :) Let's stay close together on how failure will be represented in an object (as you had proposed). Let's come up soon with a definition of the working environment (referee from where, buster from where?).

I'll keep you posted. I think the working environment will solve itself. I'm basically trying to get something solid up and running to have a less speculative discussion.

I'll try to apply my approach from ramp-resources to get something, and do my best to make it exchangeable.

Sweet

johlrogge commented 11 years ago

I hope I have something working by the middle of this week. I have pushed what I have in my referee-fork under the branch internal-assert. At the moment the internal assert is not integrated in the regular assert. There are a few issues left to solve before that works. (there are like 35 assertion failures of 2k something.

I think that I can introduce the promise approach and make exceptions work for asserts that are not deferred. In case an assert is deferred and exceptions is configured to be thrown I can detect that (neither failure nor success has been received) and fail the test as inconclusive with a request that the user turns off exception on failure or makes sure that asserts are synchronous.

My referee branch is very much work in progress. I'm thinking about renaming internal to definition and let it expose properties such as type (refute or assert) and name such as equals. Those would be useful for describing asserts. Sort of making them self documenting.

For a taste of how the assertions can be tested outside of referee check the internal assert tests. Time to sleep hours ago...

cjohansen commented 11 years ago

Interesting reading guys!

Possibly referee can still support exceptions while referee-combinators will require the eventmodel. Basically you can use what exists today but if other frameworks want to use referee-combinators they will have to bend our way (rather than the other way around).

This definitely sounds reasonable.

There are two reasons for failure-by-exceptions in referee:

  1. It's the defacto standard for assertion libraries = easier to integrate with arbitrary test frameworks.
  2. Stop on first error.

The first one can easily be solved: As long as referee uses events, you can integrate referee with an exception-based test framework very easily:

referee.on("failure", function (err) {
    throw err;
});

The second point is important. I know exceptions and async go together like oil and water, but stopping on first error is a pretty good feature. We can of course mitigate this to some extent in Buster, but not in all cases. For instance, we can only ever report one assertion failure for any given test (we already do this). We can also suppress other kinds of errors, e.g.:

refute.isNull(obj); // If this fails...
assert.equals(obj.prop, 42); // ...then this will throw an error

We handle this as well - to some extent. When this happens asynchronously, the second error may turn up as an "uncaught exception". So that's the trade-off.

Lastly, exceptions give you a stack trace, but I'm not sure how important they are for assertion failures (vs unexpected exceptions). Maybe I should educate myself on domains (I'm not sure if they're relevant, but maybe?)

meisl commented 11 years ago

Thx @johlrogge! Looking at actual code always helps me to understand better, no matter how much work-in-progress it is. I had read over

The internal assertion returns it's result (currently as a promise, more on that later)

earlier, because of the "more on that later" but now I'm starting to see the value. Failure is represented by rejection, success by resolution, and we're not even restricting ourselves to only have synchronous "internal" assertions. We could even put the progress callback to use, eg "1st sub-assertion", "2nd sub-assertion"... Well, maybe... As you can see, I'm pretty much enthused by it :)

I'm thinking about renaming internal to definition [...]

definition is more specific (good) but one might think it's the object passed to referee.add (what I had called .assertionDef in #7). Hmm, how about .raw (and referring to them as "raw assertions")?

[...] and let it expose properties such as type (refute or assert) and name such as equals. Those would be useful for describing asserts. Sort of making them self documenting.

Yes please :) as I was begging for in #7.

[...] how the assertions can be tested outside of referee [...]

ie assertion under test comes from different referee than the one used to perform the test, right? I'm asking because I saw buster is not a dev-dependency of referee - guess it should be?

meisl commented 11 years ago

Interesting reading guys!

Thanks, @cjohansen :)

2 Stop on first error

Totally agree that it's an important feature. And now I'm thinking that promise-as-internal-assertion-result is even more powerful than exceptions, in the sense of a proper superset!

We handle this as well - to some extent. When this happens asynchronously, the second error may turn up as an "uncaught exception". So that's the trade-off.

Well, "first", "second"... cease to be well-defined in an async setting. Is it textual or temporal order? (I know what you're referring to by "second error", it's to point out the general problem). We can of course decide to define it one way or the other. But I'm almost sure we can do even better! With raw assertions returning a promise - we could retain info about both orders, and by comparing them make a (more) clever decision what to present to the user.

Lastly, exceptions give you a stack trace, but I'm not sure how important they are for assertion failures (vs unexpected exceptions).

In case of assertion failure it's not the whole trace but only one stack frame (or possibly two in async), namely the location of the failing assertion (textual / textual+temporal). But that is important to have I think. However, with promise-as-raw-result we can carry with us whatever we like. We can always throw and immediately catch to get hands on the source location; there might even be tricks to avoid doing this if the assertion passes, should it turn out to bear too high a perf penalty.

Maybe I should educate myself on domains (I'm not sure if they're relevant, but maybe?)

I'm no expert but I do know they're relevant for exceptions (not urgent though). But hey, if we go entirely without failure by exception - then they're not relevant for failure :)

johlrogge commented 11 years ago

Just dropping in for a quick thank you @cjohansen and @meisl. You input is very much appreciated and very encouraging. Don't have long so I'll just answer an easy question:

ie assertion under test comes from different referee than the one used to perform the test, right? I'm asking because I saw buster is not a dev-dependency of referee - guess it should be?

Correct! The raw assertions can be tested with anything that can assert on the rejections/resolutions. In this case I use busters asserts.

johlrogge commented 11 years ago

Also, good catch about the assertion not having to be synchronous. That is the plan. Not sure what that means for @johansens's suggestion of referee.on('failure',...). I was thinking to make referee support returning a promise from the assert-function as an alternative to just true. I think it will open up pretty interesting opportunities.

Also good observations about how we can salvage stacktrace data and bring it with the promise rejection from "the source".

johlrogge commented 11 years ago

Another point: The splitting of declaring assertions and validating them will actually allow us to track how many assertions we expect to validate. That means that we can probably do away with done or having to return a promisefrom an asynchronous test. We can simply consider a test done when all assertions have been met (or timeout) - this is because the expected values are declared up front.

cjohansen commented 11 years ago

I was thinking to make referee support returning a promise from the assert-function as an alternative to just true. I think it will open up pretty interesting opportunities.

Cool, +1 +1 +1

We can simply consider a test done when all assertions have been met (or timeout) - this is because the expected values are declared up front.

What then about code that e.g. puts an assertion in the callback to fs.readFile? Do you propose to wrap the whole thing in an assertion? The done approach seems like a more generic solution to that problem?

meisl commented 11 years ago

[...] - this is because the expected values are declared up front.

oh... let's not overshoot. That is something you can - but need not - do with "partial" assertions. We should not require everybody to use it. Not even to do async tests by returning a promise. done is beatifully simple, one need not even know what promises are (or so-called "thenables", for that matter). That is a Good ThingTM.

I'm not even convinced that it should be offered as an alternative to done or return-promise. It's good to have something that indicates that it's an async test, be it done or a return ... (tests are being refactored, too...). Also, how would you ensure that the prerequisites for such auto-magic are established (not even sure what they really are)?

It's of course possible that I missed something. If so plz correct me.

johlrogge commented 11 years ago

Who said done should go? And who said everything we can do should be done. Done looks simple but I actually think returning a promise is way more simple (if one understands promises). More simple than calling done would be to not have to call or return anything at all.

I think my proposed approach of tracking "open" assertions would only work in promise scenario:

return when(readFile('file')).then(assert,combinator.contains("in the file"));

It would basically allow us to leave out return

when(readFile('file')).then(assert,combinator.contains("in the file"));

I'm not sure it can be made to work in a reliable way but lets not shoot it down before it takes off :) It will be some time before we get to implementing it anyway (if ever). In any case it could be interesting to keep track of the number of open assertions and the ones that has been resolved.

@cjohansen To use that style of tracking one would have to use a more "monadic" style of testing. Where all assertions are open before execution starts. It may never become intuitive enough. Just brainstorming...

meisl commented 11 years ago

Hey, no offense intended :) I'm totally pleased, now that I know the status you're giving it.

I find all this quite promising and exciting myself, just felt a call for "keeping the horses calm" wouldn't be too inappropriate at this point. (sorry, don't know the saying in English but you'll get the point)

cjohansen commented 11 years ago

@johlrogge I see better what you mean now. This is a very interesting approach. Probably not suitable for everyone, but certainly something I'm interested in seeing more. Thanks for explaining :)

johlrogge commented 11 years ago

I have just pushed internal assertions. I'm going to clean up my tests a bit. I have some ideas to make them cleaner. I will also rename internal assertions to raw assertions. Next I will try to implement the possibility of returning promises from assertions and also dive deeper into throw on failure...

currently all tests pass and I think we're in pretty good shape to start depending on the raw assertion from referee-combinators

meisl commented 11 years ago

Very cool :D I'll take a closer look tomorrow, bust just take the time you need for the refactorings. My announced "parametricTests" - and even more so "meta-assertions" will take a bit longer anyways. Pretty sure they'll profit from raw, nevertheless... and that you'll like 'em :)