[WIP] Improved failure reporting to include the expected/actual result

[chancancode]

If available, the reporters would now include the expected and actual result for each failed assertion. Currently, this is only supported by the qunit adapter.

Fixes #225

---

The qunit assertion API looks like this: equal(actual, expected[, message]);. When this fails, it'll pass {actual: actual, expected: expected, message: message, source: <stack trace>, ...} to the reporter. Most people don't bother putting a message in assertions like these, which is why it is often just reported as undefined in the teaspoon reporter.

I bumped into this issue when setting up teaspoon for our app, so I forked it and fixed it on a branch. This is how it looks like:

I just need to work for our app, I suppose there are other things to consider before this can be accepted (code style, look/feel, tests, changelog, etc), and I'm not sure if I'll have time to do those in the near term. If someone has the time to pick this up, please feel free to use my commit as a starting point. Would love to see this merged into master.

References:

• https://github.com/modeset/teaspoon/blob/master/vendor/assets/javascripts/qunit/1.12.0.js#L922-L927
• https://github.com/modeset/teaspoon/blob/master/vendor/assets/javascripts/qunit/1.14.0.js#L436-L441

cc @cwick @jejacks0n

[cwick]

Nice! Unfortunately, it doesn't work when the "Full Report" button is pressed. See this line in base/reporters/html.coffee. It doesn't create a FailureView if build-full-report is true.

If it's not using FailureView, there must be some other view that's rendering just the message and stack trace (while omitting expected and actual) but I haven't found it yet. Any ideas?

[chancancode]

@cwick would it be this? :smile: https://github.com/modeset/teaspoon/blob/master/app/assets/javascripts/teaspoon/base/reporters/html.coffee#L75

I also noticed that the console formatters need to be updated to show this as well... feel free to steal this and improve it :grin:

[cwick]

Ah, yes, that looks promising. I should be able to make some improvements to this next week.

[chancancode]

@jejacks0n Do you have any comments on the approach? :) Are you okay with adding expected and actual as global (but optional) properties to the error object?

[jejacks0n]

Well, I think it's a good idea, I struggle to merge it. The reason is because it should be up to the test framework (eg. qunit) to make useful messages. The reason this is exclusive to qunit is because it doesn't provide useful messages. Is there a way to accomplish this without changing teaspoon? eg. calling a message helper function that you define in your spec helper?

The reason I struggle with this is that it's a feature for only one library, and I don't want to go down that path simply for the sake of maintenance.

[jejacks0n]

The recommended solution is a custom expect method that builds the message and then calls the original expect method.

[chancancode]

@jejacks0n actually I'd say that qunit is providing more information here :)

Examples of how it works in qunit:

equal(1, 2, "I expect one and two to be equal!") // => {actual: 1, expected: 2, message: "I expect one and two to be equal!", ...}

equal(1, 2") // => {actual: 1, expected: 2, message: undefined, ...}

ok(false) // => {message: undefined, ...}

ok(false, "this fails") // => {message: "this fails", ...}

It is trivial to just construct a flat message from these in the qunit adapter. However, we would loose out on the opportunity to format it nicely.

Thoughts?

[chancancode]

If you have used qunit for a while, the "nice format" is more than "nice to have" :) I'd even suggest extracting that information from other frameworks even, you guys are missing out ;)

[jejacks0n]

To be clear, you're saying that qunit is able to provide more information, not that it in fact does usefully from the start.

Forgive me here, I'm just pseudo coding, but you can tell me what's wrong with this two line fix that goes in your spec helper.

my_equal = (actual, expected, message) ->
  equal(actual, expected, "Actual: #{actual}\nExpected: #{expected}\n#{message)")

This is what I think it should be if you want that support. Do you see why I would be apprehensive about merging something that only works for one framework?

[edit] I used expect, not equal.. my bad.

[jejacks0n]

@chancancode actually, no, I really dislike the lack of nesting, pending, and expressiveness that the spec frameworks have. Take a look at a mocha spec and see what I mean. They convey much more than I can accomplish in a "test_something" function name.

[jejacks0n]

Also, I use rspec, so the specs read the same between ruby and javascript.. if I used mini_test or test_unit, I might agree with you though.

[jejacks0n]

Don't get me wrong, I appreciate your work. It's just hard to merge something that I won't be able to track easily.

[chancancode]

@jejacks0n don't worry, I'm not trying to "convert" you to start using qunit, or argue that it is "better" :smile:

My last comment was strictly referring to the fact that it provides more information for the formatter to work with, and that I would suggest extracting those same information (i.e. actual, expected) out of other frameworks where possible :)

The reason why it is a good idea to provide those information for the formatters to work with (instead of flattening those into a string) is to possibility to format those nicely.

In the case of the HTML formatter, it made it possible to display display the content of actual/expected in fixed width font and to call "inspect" on them (the current implementation JSONifies it) for example.

This is helpful when, for example, actual is "omg " and expected is "omg".

(To be fair, the "inspect" part is achievable even if we do flatten it. However, the fixed-width font part wouldn't be possible.)

To help you understand what the default qunit reporter was able to do with those extra information, here is a screenshot from the original issue:

[chancancode]

Theoretically, the adapters could also just concat the stack trace into the message and just return a flat string as well :) This would work fine for the current formatters (the HTML one at least), but it would restrict the kind of formatters you will be able to write.

[chancancode]

(To be clear, this doesn't change how other framework adapters work currently – if the expected and actual fields are absent, the formatter simply ignores it and renders the same output.)

[jejacks0n]

A few things if this is to be merged then.

• inspect should go away. I would prefer that JSON call be inline.

• we need to call #htmlSafe on the expected and actual -- even if it's in a code, it can still break layout if it includes html, also information can get lost that way.
• there's an absence of specs for this.
• this should happen for the console reporter as well.
• it should be used when the full report is enabled since that mode is the default.

[chancancode]

Sounds reasonable :) Just want to check if you're okay with the overall feature before we proceed any further. Thanks for spending time reviewing this :heart:!

@cwick, if you have time to follow up on this, please feel free to take it from here :grin: :heart: Otherwise I'll add this to my queue and get to it in the next (few) week(s). :see_no_evil:

[chancancode]

(By the way, JSON.stringify is probably a super naive implementation of inspect that only works for basic primitive types)

[jejacks0n]

JSON.stringify is naive, and will error if it's an instance that's self referential etc. That's partly why I think the test framework, or assertion library should handle that stuff. Displaying a diff is much more useful than actual vs. expected, and to me that should be part of the message, but since qunit doesn't do it nicely, it seems fine to add it.. though, I might suggest as a follow up, that we put it only in the qunit subclasses and not the base class.

[briandunn]

What is the status here? How can I help?

[jejacks0n]

you're free to try it out and let me know how it works for you.

the problem I see with it, and the reason I didn't merge is the JSON stuff.. it seems too brittle and would likely cause additional support issues.

[chancancode]

What Jeremy said. And the console output is missing too iirc. Feel free to steal this code and resubmit an improved version :grin:

For the record we are still using teaspoon with this patch at work, and it's still working great for our purposes :+1:

[CiaranMcCann]

Is this going to get merged?