Allow calls to "expect" within a single test to be cumulative

[amb26]

Right now it hurts modularity that it is not possible for a test function to call various subfunctions which each make their own separate calls to "expect" along with issuing some assertions. Forcing the user to push the "expect" count to top level makes test cases fragile - it would be helpful if several calls to "expect" within a single test case were cumulative, rather than the last one replacing the total count.

[jzaefferer]

The patch is simple enough, but the design is questionable. Having a single expect per test means that you have to structure modules, tests and assertions in a way that a single expect makes sense. Having one test method call out to several other methods, each with expect calls, sound like you're not structuring your tests in a way to fits QUnit.

Before closing or landing, I'd like to see your usecase to get a better idea what you're trying to solve.

[amb26]

Hi Joern - this facility is essential when tests are written which are in some way reflective or driven by declarative configuration. Here is a simple setup which on the first line sets up 3 component types to be tested in an array. The body of the test iterates over the array and issues a test case for each member against a different type. The expectation is that the set of components under test can be driven directly by adding or removing elements from the head array - it would be onerous to expect the test maintainer, in order to arrive at a valid test, to inspect all the code paths and add up the total number of "expect" counts to place in the head test.

As it happens in this case the number of expects is constant (1) for each component, but we also have cases where the test cases themselves are also polymorphic, variable on the type of the component being tested, and so the burden of counting "expect" clauses would be particularly onerous.

In terms of structuring our tests to fit QUnit, I hope that in the final analysis that QUnit can be found to be useful in a broad community and being purposed to fit a diverse collection of needs, of course without compromising its core architectural principles and consistency. I hope you agree that this is an useful improvement which increases QUnit's coverage without compromising its consistency.

fluid.tests.gradeTestTypes = ["fluid.tests.gradedComponent", "fluid.tests.autoGradedComponent", "fluid.tests.ungradedComponent"];

function testEvent(message, component) {
jqUnit.expect(1);
component.events.anEvent.addListener(function() {
jqUnit.assert(message + ": Event fired");
});
component.events.anEvent.fire();
}

fluidIoCTests.test("Grade resolution test", function () {
    fluid.each(fluid.tests.gradeTestTypes, function(typeName) {
        var that = fluid.invokeGlobalFunction(typeName, ["#pager-top"]);
        testEvent("Construction of " + typeName, that);
    });
});

[jzaefferer]

In this case, why not use a test for each type?

fluid.each(fluid.tests.gradeTestTypes, function(typeName) {
    fluidIoCTests.test("Grade resolution test for " + typeName, function () {
        var that = fluid.invokeGlobalFunction(typeName, ["#pager-top"]);
        testEvent("Construction of " + typeName, that);
    });
});

Then put an appropiate module() call in front of that.

[amb26]

I think we should be free to issue tests of the granularity we prefer. It seems unnecessarily bulky to declare a new module around this construction and a fresh test case for each fixture. The inability to cumulate "expect" forces each leaf function in the testing architecture to become its own test case which is not usually an appropriate choice. I can quote more elaborate test cases than this one where the economics are even more skewed.

Could you explain your opposition to this feature, which clearly meets a need by some of your userbase, for which you have a patch, tests and a clear explanation, and breaks none of your existing tests?

[jzaefferer]

A general opposition of new features is pretty healthy for open source projects. In this case my concern is about the change in the semantics of expect. The unit tests don't tell us what code might break elsewhere due to a change, even if its very unlikely to break something in this case.

This isn't the first request asking for different module/test/assertion semantics, and I have to make sure that the usecase is actually valid.

I can quote more elaborate test cases than this one where the economics are even more skewed.

I'd like to see those, as the previous example itself wasn't convincing. Is there another (GitHub) project I can look at? Maybe there's an alternative solution to the problem.

[amb26]

Here is a more elaborate example.

https://github.com/fluid-project/infusion/blob/master/src/webapp/tests/component-tests/uploader/js/UploaderTests.js

At lines 566-577 we set up a bunch of tests, which does indeed issue a distinct test for each loop entry as you advise - however, as the tested code is polymorphic, as well as being composed of a number of independent units, the number of "expect" calls can't be easily computed in advance. We don't actually issue any expect calls here since there aren't any dynamic code paths within the tests themselves, but this is just an accident of how these tests are written, and they may well need to be expanded to issue "expect" in the future. For example, look at the "leaf function" "checkUploaderArgumentMap". I think it is unreasonable to expect this to be a whole test case in itself. It should be able to express its own "expect" count (2).

In general, I think your solution isn't workable because it forces us to couple the unit of the test fixture (which amongst other things, is coupled to the cycle of replacing the "#main" markup and setup/tearDown) with the smallest unit of code which we have which issues assertion calls. This prevents tests from being expressed in the scalable manner that one would use when writing normal code. The "teardown/replace" boundary in the architecture is a fixed one depending on the actual lifecycle of the component being tested, and should not be used to define architectural boundaries in the structure of the code forming the test cases.

[jzaefferer]

Thanks for the full example. I'll need a bit more time to get back to this, but expect the patch to land sometime soon.

[amb26]

Cheers, Joern - thanks for your patience with the discussion

[Krinkle]

I oppose the idea of cumulative expect(). It sets wrong expectations.

If you don't know the number of tests to be run in advance, don't use expect(). Counting your tests and calling expect() is useless, that's like asserting 1 + 1 to be equal to 1 + 1.

If you use a wrapper for creating several tests, (in that your assertions are not directly inside the test() callback), then you may find it useful to use incremental steps in the expect. In that case, perhaps take one of these three approaches:

• Split it up into multiple test()'s. So instead of having

test('foo', function () {
    expect(10);
    K.Test.doTenTestsWith('X');

    expect(10);
    K.Test.doTenTestsWith('Y');
});

.. you'd have:

test('foo X', function () {
    expect(10);
    K.Test.doTenTestsWith('X');
} );

test('foo Y', function () {
    expect(10);
    K.Test.doTenTestsWith('Y');
} );

Or even make the call to test() part of the doTenTestsWith method. So you only have to call that.

• Simply don't use expect(). Remember, it is optional. For example, if you are doing an ajax request and getting an unknown number of properties back (and the number is not significant), and then looping through and doing a validation on them equal( typeof x[y], 'string', x + ' is a string' );, then it doesn't make sense to use expect() because the number of tests doesn't mater. If it can't arrive at a wrong number, then don't use expect() just for the heck of using it.
• The call to expect() doesn't have to be at the start. So, if you must, perhaps use a counter. That way it is clear what you're doing.

test('foo', function () {
    var expected = 0;

    K.Test.doTenTestsWith('X');
    expected += 10;

    K.Test.doTenTestsWith('Y');
    expected += 10;

    expect(expected);
});

If we decide to keep not-supporting cumulative setting of expect, then we should also be more strict about it and display warnings if it is called multiple times.

[amb26]

Hi Timo - I disagree with your argument, for reasons which I have already stated. To deal with each of your presented choices,
i) you say - "split it up into multiple test()"s - this is unacceptable, since as I pointed out earlier,
"In general, I think your solution isn't workable because it forces us to couple the unit of the test fixture (which amongst other things, is coupled to the cycle of replacing the "#main" markup and setup/tearDown) with the smallest unit of code which we have which issues assertion calls."

ii) Simply don't use "expect()" - this is a choice we exercise from time to time, but sometimes it is important for the integrity of a test that it has run to conclusion (if it has unexpected exit paths in the logic that allow it to terminate without failure). Your argument about doing an AJAX request and getting an unknown number of properties is invalid - an AJAX request could never be part of a unit test fixture since it would introduce an environmental dependency. Of course, jqUnit is equally useful for integration tests, but unit tests are, culturally, its primary use case - as its name suggests.

iii) Arguments based around expecting the test case author to mock up this feature of the testing framework manually whenever they want it are unhelpful. The testing framework has this feature (the ability to count the number of assertions) and it would be most helpful for it to be upgraded so that it covers more use cases for the user base.

[scottgonzalez]

This is a breaking change, so I'm against it for that reason alone. But, I see two sane ways to accomplish what @amb26 is asking for:

1. Have the test helpers return the number of assertions:

test( "do lots of stuff", function() {
    var expected = 10;
    expect( expected );

    // do stuff locally which causes 10 assertions

    expected += someHelperWithUnknownAssertions();
    expect( expected );
});

1. Expose the number of expected assertions for the currently running test:

test( "do lots of stuff", function() {
    // ...
    someHelperWithUnknownAssertions();
});
function someHelperWithUnknownAssertions() {
    expect( QUnit.currentTest.expected + 5 );
    // do assertions
}

[jzaefferer]

That would be expect( QUnit.config.current.expected + 5 )

[jzaefferer]

We could make expect() return the current number of assertions when called without arguments.