TestHelper should generate separate tests

I have been starting to integrate raw assertions in combinators. I run into some inconveniences with the testhelper so I thought I'd jot them down in case some of the issues are unknown:

The TestHelper geneates assertions with custom messages for many of the cases
- If they were first class named tests they could be rerun by specifying part of the name when starting buster buster test -e node <part of test name> to run only those tests that contain .
- it may me a good idea to be able to name at least part of the name of the test explicitly. Perhaps optionally.

Some of the tests are checking aspects of the assertion that the user has no control over, things that referee-combinators do for every assert. If it works for one it works for all of them. I'm not sure what my opinion on that is. To automatically generate exhaustive tests for that or to check that aspect once. Now there is a lot of noise to check whether all assertions have a proper displayName for instance. In combination with the tests not being first class tests it is hard to zoom in on just one problem. I'm leaning towards checking those things once in a basic test for how combinators behave when extending referee and focus the tests for individual assertions on the specifics of those assertions only. Right now the specifics sort of drown in the noise

Just my thoughts, I'm putting it here for discusion

Part of my issues were because of the npm-depended buster in the project. Apparently it just ignores every argument after -e node. Still would like to discus the noise though :P

I think you're quite right - even though I'm not sure which tests/TestHelper you're referring to exactly :)

So in order to not answer questions that you didn't intend to ask:

By TestHelper you mean the function makeTests in test/test-helper.js, right?
The noise about checking displayName, in which file do you find it most troublesome?

It's not at all to put down anything - rather on the contrary. In fact I have to restrain myself because you definitely spotted important points. It'd be just too speculative for me right now.

Apparently it just ignores every argument after -e node

How are you running it?

buster test -e node 'part of testname'

When I do npm install in referee-combinators and run buster test -e node -r specification it even ignores the -r specification part.

If I remove node_modules/buster and (i guess) run my global buster the -r specification and 'part of test name' work.

My guess is you're running a mix of global and locally installed Buster. Run it like so: ./node_modules/.bin/buster-test

@meisl Sorry I got back to you so late. My main issue seemed to be that buster ignored parameters in this case. Probably due to a mix of buster-versions. I was talking about the TestHelper in referee-combinators but the important principle that remains in this issue is wether or not we separate the responsibilities of the framework referee-combinators in this case and the details about custom assertions. I didn't look into the details of the implementation but it seems like a good principle to me to test what the framework does separately from the custom assertions. The tests for displayName are an example of testing an aspect of the framework via tests for assertions. I don't know if there are more examples.

I thought to start implementing failure messages for the combinators. I thought to write the tests for that outside of the TestHelper not to interfer with your work too much. I have already implemented raw assertions for combinators. I will push them soon.

I was talking about the TestHelper in referee-combinators but the important principle that remains in this issue is wether or not we separate the responsibilities of the framework referee-combinators in this case and the details about custom assertions.

Yes, I also think that these are essential questions: which are the responsibilities of what part / what to separate from what. That's why I said you definitely spotted important points. But please, I need you to be more specific. TestHelper = makeTests in test/test-helper.js? Or rather the whole concept of having a big file test/test-helper.js plus its tests, test/test-helper-test.js??

So far, I can only answer this (reasonably): it's all very much work in progress. Almost nothing to be taken as carved into stone. I had told you that "it needs more care".

[...] and the details about custom assertions.

Again, could you please name them? That is: which details? Which custom assertions? I can only guess that you might mean my temporary (!) isPlainObjectOrFunc and isProperSubset...? Or maybe more generally, anything that can be added via referee.add?? (by "the user"? by "us"?) These have very different statuses - and there is more than one viewpoint from which to define "status". I just cannot go through all possible interpretations I can think of and comment on each, speculatively. Sorry.

[...] it seems like a good principle to me to test what the framework does separately from the custom assertions.

Yeah, sounds very reasonable to me, too. Alas, it must be made more concrete. That is, without (me) knowing more precisely "what the framework does" vs what "custom assertions" are - it's no more than what I totally agree with - and done...!

The tests for displayName are an example of testing an aspect of the framework via tests for assertions. I don't know if there are more examples.

First off, having displayName is crucial for higher-order assertions. At least two things: for making failure messages and for synthesizing names of generated tests (as makeTests does and parametricTests will).

Then: makeTests does create tests for that, and _they are separate tests_. They are created dynamically, however, so that might pose a problem with selecting them via buster test -e node <part of test name> but I don't know. In fact I was assuming that buster would be "smart enough" but didn't try it at that stage. Simply because it's a special use case of which to take care is just useless as long as things are in the floating as much as now. But since you seem to have tried it, have you managed to get it working?

Additionally: makeTests creates the tests for displayName _automatically_. Could you plz try a text search for "displayName" in test/referee-combinators-test.js? ... ... sorry, I just don't get the point of

Right now the specifics sort of drown in the noise

I really need you to tell me. And I will listen, promised. It's just that I don't get it.

I thought to start implementing failure messages for the combinators. I thought to write the tests for that outside of the TestHelper not to interfer with your work too much. I have already implemented raw assertions for combinators. I will push them soon.

Good plan :D We can always throw out then-obsolete stuff. Probably mine, as I've set up quite a number of improvised things and tackling it at the level raw assertions looks like the right thing. Also, even though it might look like doubled work at first glance, I'm pretty sure that surveying two sources will be far better than going through the hell of conflicts in git merges.

So, to summarize: no offense, just don't leave me at guessing. I am interested in your explanations - and I need them :) And I apologize for not yet being able to push what I had announced. We'll see.

I'll try to be more concrete thus time

separate tests

Not an issue, I was mislead by my messed up buster config and drew the wrong conclusions about what the problem was. Taken care of. I can now filter out individual tests like I want without making any changes in test-helper.js. So lets not focus on the separate test part, ok?

custom assertions

The assertions referee-combinators add. Currently attr and structure.

noise

IMO this is noise (explanation follows after):

$ node_modules/.bin/buster-test -e node -r specification .displayName
combinator ('partial') assertions
  ✓ derived from built-in binary equals assert.equals [raw] should have proper .displayName
  ✓ derived from built-in binary equals refute.equals(42) [appliedOnce] should have proper .displayName
  ✓ derived from built-in binary equals assert.equals(42) [appliedOnce] should have proper .displayName
  ✓ derived from built-in binary equals refute.equals [raw] should have proper .displayName
  ✓ derived from built-in binary greater assert.greater [raw] should have proper .displayName
  ✓ derived from built-in binary greater assert.greater(23) [appliedOnce] should have proper .displayName
  ✓ derived from built-in binary greater refute.greater [raw] should have proper .displayName
  ✓ derived from built-in binary greater refute.greater(23) [appliedOnce] should have proper .displayName
  ✓ derived from custom binary assert.equalsTwoWithCoercion() [appliedOnce] should have proper .displayName
  ✓ derived from custom binary refute.equalsTwoWithCoercion [raw] should have proper .displayName
  ✓ derived from custom binary assert.equalsTwoWithCoercion [raw] should have proper .displayName
  ✓ derived from custom binary refute.equalsTwoWithCoercion() [appliedOnce] should have proper .displayName
  ✓ derived from built-in unary isTrue assert.isTrue() [appliedOnce] should have proper .displayName
  ✓ derived from built-in unary isTrue refute.isTrue [raw] should have proper .displayName
  ✓ derived from built-in unary isTrue assert.isTrue [raw] should have proper .displayName
  ✓ derived from built-in unary isTrue refute.isTrue() [appliedOnce] should have proper .displayName
  ✓ extension - structure 1 level assert.structure [raw] should have proper .displayName
  ✓ extension - structure 1 level assert.structure({ key: "value" }) [appliedOnce] should have proper .displayName
  ✓ extension - structure 1 level refute.structure({ key: "value" }) [appliedOnce] should have proper .displayName
  ✓ extension - structure 1 level refute.structure [raw] should have proper .displayName
  ✓ extension - structure 2 levels assert.structure({ key0: { key1: "value" } }) [appliedOnce] should have proper .displayName
  ✓ extension - structure 2 levels assert.structure [raw] should have proper .displayName
  ✓ extension - structure 2 levels refute.structure [raw] should have proper .displayName
  ✓ extension - structure 2 levels refute.structure({ key0: { key1: "value" } }) [appliedOnce] should have proper .displayName
  ✓ extension - structure explicit assert for attribute assert.structure({ enabled: function assert.isTrue()() {} }) [appliedOnce] should have proper .displayName
  ✓ extension - structure explicit assert for attribute assert.structure [raw] should have proper .displayName
  ✓ extension - structure explicit assert for attribute refute.structure [raw] should have proper .displayName
  ✓ extension - structure explicit assert for attribute refute.structure({ enabled: function assert.isTrue()() {} }) [appliedOnce] should have proper .displayName
  ✓ extension - attr 1 level assert.attr [raw] should have proper .displayName
  ✓ extension - attr 1 level assert.attr("key", assert.equals("value")) [appliedOnce] should have proper .displayName
  ✓ extension - attr 1 level refute.attr [raw] should have proper .displayName
  ✓ extension - attr 1 level refute.attr("key", assert.equals("value")) [appliedOnce] should have proper .displayName
  ✓ extension - attr 2 levels assert.attr [raw] should have proper .displayName
  ✓ extension - attr 2 levels assert.attr("key0", assert.attr("key1", assert.equals("value"))) [appliedOnce] should have proper .displayName
  ✓ extension - attr 2 levels refute.attr [raw] should have proper .displayName
  ✓ extension - attr 2 levels refute.attr("key0", assert.attr("key1", assert.equals("value"))) [appliedOnce] should have proper .displayName
util
  ✓ format with anon function that has own prop .displayName
  ✓ format with named function that has own prop .displayName

While displayName is an important aspect of asserts the rules for generating them are the same. It is an algorithm that referee-combinators applies when an assertion is added (and for assertions that has already been added when referee-combinators are loaded). It is a crosscutting concern that is true for all asserts and the rule is no different for attr, structure, equals or any other assertion we can come up with.

Therefore this should tested once for referee-combinators and not be repeated for every assertion we do makeTests for.

I think it should be part of this output:

$ node_modules/.bin/buster-test -e node -r specification basics
combinator basics
  ✓ combinators have raw assertions
  ✓ referee has combinators
  ✓ combinators have assertions
  ✓ armed combinators have raw
  ✓ combinators raw assertions can pass
  ✓ combinators raw assertions can fail
1 test case, 6 tests, 6 assertions, 0 failures, 0 errors, 0 timeouts

All of the above could be added in makeTests but that would make no sense IMO. I think it is clearer to describe the behaviour in one place. The basics (not married to name... fundamentals?) describes what referee-combinators does on a framework level. Then whe also need to describe each new assert we add. I think it is important that the tests for each assertion focus on what that particular assertion brings to the table. everything else is noise in that context.

I hope I was clearer this time :)

Oh man, thank you! That helps a lot. I was really thinking that separate tests (or the lack of) is the issue... Maybe change title / put an "EDIT:..." into initial post?

Re the noise: again, I didn't think about that :) It's because I usually do not look at which tests passed but rather only the ones that failed. So what I focussed on with generated tests was keeping the test code concise plus give enough info for the case of failure. For the latter I see basically two strategies:

put as little assertions into a (generated) test as possible (ideally 1), and put the info into the test name
generate less separate tests and put (more of) the info into the assertions' messages

The second would have produced less noise for "your" use case, but it's harder - even more so if you don't have raw assertions. "Harder" wrt to getting equally informative messages in case of failure ("my" use case). So that's why I had chosen the first path. But the main point here is that it's a trade-off.

I do agree on separating cross-cutting concerns (aka framework-level) from details of a particular assertion ("what it brings to the table", particularly). Only that I'm not yet clear about how to best achieve this. You know, coverage is a concern, too.

However, separation was in fact NOT my concern when writing makeTests. That's not to say that it's not important - on the contrary! But at that stage, in order to check such "framework invariants", the easiest thing was - to just produce a separate test for each and every one. Note that I did have in mind that those things might very possibly be handled differently in the future, so I still think it was a good decision - simply because throwing away not-so-clever stuff is easier. Well, relatively "not-so-clever" stuff... :)

Now, what next?

Let's check the very concept of generated tests. Pros/chances are coverage and less clutter in the test code (noise in a slightly different sense); cons/risks are more cryptic test code (cryptic not because of clutter but too much of things you haven't seen before) and noise in your sense/use case. What else? Let's put this into a wider view, ie not just makeTests as it is and we use it now. ~> parameterized tests

Then: our different "use cases", as I called it. As said, "yours" was really under my radar. But it is definitely a valid one. For example, if you take the tests as executable documentation - the ideal - then first looking at all the test names to "see what it's all about" should be a good idea. Alas, as you pointed out, this can easily be subverted...

It's all trade-offs, the art is in the weighing. As for "your problem", the noise in test results, I could vaguely imagine some customization. But it must be a "one-stop-shop", one central switch to choose between more tests with less assertions each vs less tests with more assertions each. Just a thought.

Plz note that whether / how we do "the separation" is not exactly the same question. It does share the criteria, though.

What are your thoughts?

I'll have to take a closer look at what you pushed recently, re combinator basic tests. But it'll take a while, I currently don't have much spare time.

Edit title

Yes, I this issue started gliding from my wrong assumption to something else. Edited

Assertions per test

I'm a strong proponent of one assertion per test as in a test should only have one reason to fail. When I say one assertion per test i really mean one logical assertion or one thing per test. I'm hard pressed for non contrived examples for this, mostly it means just one assert.x(a, b) per test but sometimes it may be clear to verify a with several asserts. The point is that those asserts should strengthen the proof of the same thing.

I got a feeling that my point came across differently.

I'm not against lots of tests in the output as long as they are relevant to the context. As a rule of thumb: any test we can generate with no other input than an assertion (combined or not combined with an expected value) does not belong with tests for a specific assertion

Examples:

correct fail( assertion, "expeted", "different actual")
correct pass(assertion, "expected", "expected")
correct message(assertion, "expected", "actual", "expected <'expected'> but was <'actual'>")
wrong hasDescription(assertion)

I wrote the assertion explicitly here for clarity. I think the way pass and fail as implemented in make tests is good. It reads really well to provide the assertion and the parameters and then list the specifics for pass/fail/message in the body.

Actually, I think we could maybe name makeTeststo something more "DSL"-like:

the(assertion(expected)).will(function(passFor, failFor, yieldMsg){
   passFor(expected);
   failFor(other);
   yieldMsg(other, "was other");
});

I'm not quite happy with what the above looks like but I think it communicates the idea of making the tests read like documentation and put the focus on the specifics of the given assertion.

Coverage

I don't think this is a trade off, it is a question of how coverage is measured. The way I see it the function that generates a description given an assertion/refutation is not any more covered if we test it for each assertion we test than if we test it once and for all separately and cover all paths of the function that generates a description. Ie:

A description of an assertion
- contains the word assert
- contains the name of the assertion
- with an expected string
- it contains the string
- with an expected object with a name
- it contains the name within {}
- with an expected function
- it outputs the name of the function
- it ...

I just made up examples here. The point is how assertions are described can be described clearly and explicitly once and for all. We sort of give the rules and yes, I do value the documentation aspect of tests too.

If we want to cover all generated descriptions we can generate tests for the descriptions:

testAllDescriptions(referee)

Separate from the tests about tests about assertions (such as attr and structure). I don't think those tests would add much value but that is how I would prefer ensureing that all descriptions are correct rather than piggybacking on the tests for each assertion.

IMO tests that belong to an assertion are:

examples of values that give a failure
examples of values that succeed
examples of messages for different failures.

I think that part of makeTests is excellent and really gives a lot of value and readability of tests.

configurable many assertions per tests vs one assertion per test

Preferably not. I think we just need to come up with a consensus we are happy with and go with that.

Thanks, I've really got the feeling that the discussion is now getting "to the meat". Alas, I must ask for a little more of your patience, for me to think about all your good points :)

busterjs / referee-combinators