todo property on assertions feels incorrect

[keithamus]

In the current readme, assertion objects have a todo parameter - indicating whether or not this was part of a todo test. This feels invalid for a few reasons:

• As a "separation of concerns" principle, the assertion should not know about its parent state.
• More concretely to the above bullet point, assertion libraries such as chai cannot know about the test state.
• This doesn't seem to cover all states in a test, e.g. skipped isn't captured info in an assertion error. If skipped isn't, why is todo?
• Existence of the property on the assertion object implies some assertions (within the same test suite) can be todo while others cannot.
• This violates the principle of single source of truth. What happens if my test object says its state is "todo" but my assertion object says todo: false? Which one do I believe?

My suggestion is to remove the todo property from the assertion object, and have it only exist as a state on the test object.

[trentmwillis]

This reasoning seems sound. It was added to assertions data originally because the first implementation within QUnit required it to be present due to the way some of the test reporters work. That said, it should not be a required field due to the reasons you list above.

[Krinkle]

Proposing we make this optional (or simply remove) in the next major release.

I'm interested to know what makes it hard to track in Chai. I completely agree on the separation of concerns and that Chai shouldn't have to track this state. I'm just curious as to why it would be difficult, as that might lead us to uncover a deeper compat issue or difference to be aware of and for us to document internally.

In particular, because consumers of js-reporters do need to know this information about each high-level test wrapper and will generally have that information available indirectly for each assertion, so that they could e.g. generate a nested or flat results lists as they wish where any test-information is repeated (e.g. by prefixing the suite names and test names, and their skipped/todo status etc.). Would that not be possible either?

@keithamus Could you elaborate on why Chai can't know about this state?

[Krinkle]

I propose that we simply remove it entirely from assertion object. The spec allows additonal non-standard properties to exist, so this would be a backwards-compatible change to make.

Even on QUnit it is always decided per-test, it cannot be set per-assertion.

One thing to keep in mind is how reporters need to read the "todo" status. I recently rediscovered karma-runner/karma-qunit#111 which suggests at least in the past there was confusion over how a reporter is meant to consume "todo" event data - where Karma currently wrongly fails the build due to perceiving the todo failures as real failures.

[keithamus]

@keithamus Could you elaborate on why Chai can't know about this state?

Chai is an assertion library, not a test runner. It has no knowledge of the test runner under execution; it could be jest, ava, mocha, even QUnit. The only way for Chai to know if a test is todo is for users to somehow pass that state to it on a per-assertion basis. Chai is essentially just a bag of functions that throw errors if the given object does not meet expectations.

[Krinkle]

@keithamus Thanks, that makes perfect sense indeed.

Do you see Chai or other assertion libraries as potentially implementing some part of the CRI spec?

Perhaps this was obvious, but I had not previously thought of the "Assertion" event data (emitted by test runners), and the assertion exceptions (thrown by Chai) as potentially being one and the same. As said, todo definitely should go, and I see now how that effectively means a test runner could likely transparently forward the caught exception as "Assertion" event data into its testEnd event.

It may still need a mapping step for other sources of errors such as uncaught non-assert runtime errors, rejected promises, manually recorded errors (QUnit), or from non-conforming assertion libraries. But it'd be pretty cool not to need that in the general case.

[keithamus]

@Krinkle the desire is the next major version of Chai (v5) will implement 3.5 Assertion events as part of the CRI. Any failed assertion will emit an assertion event (and if there are no listeners to that event it'll throw). In a CRI world, I would expect assertion library agnostic runners such as Mocha to simply reflect events coming from assertion libraries like Chai - in other words they wouldn't create Assertion event objects, merely listen to the assertion libraries events and re-dispatch them on their own event bus.

Although I'm not an author of a test runner library, I would imagine for uncaught non-assert runtime errors, marking the TestEnd event as failed would be sufficient. Perhaps it should be allowed that errors is an Array<Assertion | Error> meaning that non-assertion errors can be added to the errors property? This is drifting from the topic though.