-
Notifications
You must be signed in to change notification settings - Fork 19
Closed
Milestone
Description
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 saystodo: 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.
jzaefferer and Krinkle
Metadata
Metadata
Assignees
Type
Projects
Milestone
Relationships
Development
Select code repository
Activity
trentmwillis commentedon Dec 23, 2017
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 commentedon Sep 17, 2020
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 commentedon Sep 24, 2020
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 commentedon Sep 24, 2020
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 commentedon Sep 24, 2020
@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 itstestEnd
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.
Spec: Remove "todo" from Assertion event data
Spec: Remove "todo" from Assertion event data
Test: Tolerate additional properties on Assertion event data
keithamus commentedon Sep 25, 2020
@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 anArray<Assertion | Error>
meaning that non-assertion errors can be added to theerrors
property? This is drifting from the topic though.Spec: Remove "todo" from Assertion event data (#119)