Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTML reporter: Allow the error to contain a formatted HTML error message #1349

Merged
merged 2 commits into from Jun 5, 2015

Conversation

papandreou
Copy link
Contributor

For the next version of the Unexpected assertion library we're focusing on adding colored object diffs to error messages in "deep equal" and related cases. We have reached the limits of mocha's current approach with serializing err.actual and err.expected as JSON and then doing a line-based diff. It imposes too many restrictions on the quality of the output, especially when attempting to shoehorn other types of nested diffs (eg. binary Buffer or Uint8Array) into the resulting output.

Also, we have implemented a lightweight type system that allows "userland" to introduce new types and control exactly how they they equal, inspect and diff. It's awkward to make that interoperate with the test framework rendering the diffs.

When running in node.js nothing is preventing us from omitting err.actual and err.expected and including an ansi-colored diff in err.message. We have that working fine already. We also want this to work in the browser, though, where mocha currently doesn't render diffs at all for some reason. We found that the simplest way of accomplishing that in mocha-land is for the HTML reporter to look for an HTML-formatted message on the error object and use it if available.

What do you think, is this something you would consider merging in?

Alternatively, the type system and all formatting of output could be adopted by mocha and maybe other test runners, but we're afraid that would be a premature standardization that would make it too cumbersome to iterate the concept further.

Here's a showcase of some of the diffs we have implemented so far (we temporarily removed the scrollbars for a better overview):

unexpectedobjectdiff

unexpectedhexdiff

unexpectedfunction

@jbnicolai
Copy link

Very much interested in this! I'll look at this more thoroughly this weekend, and am hesistant about adding any more 'bloat' to mocha, but I lovethe way the diff output looks for different types.

@boneskull
Copy link
Member

interesting

was just looking @ some of the diff code because it handles dates poorly.

@papandreou
Copy link
Contributor Author

@jbnicolai Had the time to look this over?

When you think of it, this could pave the way to actually remove cruft from mocha by getting rid of the diffing code. That is, if there's an agreement that this task is better solved by the assertion library.

By the way, we have tested the code that splits up the error message and the stack trace in the legacy browsers that the code has workaround for, including IE7 and Safari 5.

@boneskull boneskull added the status: waiting for author waiting on response from OP - more information needed label Oct 17, 2014
@papandreou
Copy link
Contributor Author

@travisjeffery @boneskull @jbnicolai Could someone please take a look? To see it in action you can install the newest beta of unexpected 5 (presently 5.0.0-beta12) and try it out in the browser with this commit included, or in the terminal with a stock mocha.

If none of you really have an interest in the subject, either @sunesimonsen or I could lend a hand with maintaining it. Doesn't seem like you need more people on the board right now, though :)

@boneskull
Copy link
Member

@papandreou

Don't know when/if @jbnicolai will have time to look @ this. So I have a couple things.

  • Browser support:
    • Looks like there's some CSS3 stuff in there. How does it degrade?
    • Tell me about htmlMessage. What browsers support that? What happens if it doesn't exist?
  • Please squash commits
  • Please see draft contribution guidelines.

If you are interested in maintaining a reporter, I'd like to see a proof-of-concept of using it as a separate module. Modularizing Mocha, and taking the reporters out of the core, is a direction we'd like to head in. If you want to do this, create a repo, and once it looks good, we'll put it in the mochajs org and make you the owner of the repo. The Mocha core will need to be patched as well to read external reporters. I can assist you with this bit if you like.

Pie-in-the-sky: use a templating language for this reporter.

@papandreou
Copy link
Contributor Author

@boneskull Thanks for the feedback!

Looks like there's some CSS3 stuff in there. How does it degrade?

The CSS3 stuff introduced by 5e695c7 was copied from the #mocha .test pre selector in mocha.css, and we've verified that it degrades well all the way down to IE6.

Tell me about htmlMessage. What browsers support that? What happens if it doesn't exist?

No browsers support it, it's an extra property we propose adding to Error instances thrown from the assertion lib (like showDiff, expected and actual) so that the test runner can pick it up to get a "higher fidelity" HTML error message. That's where this PR comes in -- it makes mocha use the htmlMessage if available, and otherwise it falls back to the current behavior, ie. rendering the message property as plain text in a <pre>.

We've also thought about other options, such as making htmlMessage a function so it can be computed lazily, or even exposing our intermediate format (https://github.com/sunesimonsen/magicpen), which can be rendered to either html, plain text, or plain text with colors (using ansi codes). However, those options make the API more complex, so we went for the simplest thing we could come up with. If you'd prefer another way of doing it, we're more than willing to adapt.

Please squash commits

We intentionally kept the two commits separate. Even if you decide against supporting this feature, we recommend cherry-picking the first commit (a0f0488) since it's a nice stand-alone refactoring that makes it easier to follow how the error message and stack trace are extracted in different browsers.

@boneskull
Copy link
Member

@papandreou Would PR #1357 have any impact on this PR?

@papandreou
Copy link
Contributor Author

@boneskull No, they're not conflicting. You're improving the built-in diffs, which aren't even shown by the html reporter currently.

Nice effort btw. I'd be happy to weigh in if you get even more frustrated and find yourself wanting to improve the diffs even more :)

@papandreou
Copy link
Contributor Author

We're still eagerly awaiting feedback on this. At this point this issue should be thought of as a place to discuss how to add colored messages and diffs in the browser. The fact that it includes a PR is just meant to demonstrate our position on how to accomplish it.

By the way -- I'm not sure how to add a test for this behavior in html reporter as it currently doesn't have any.

In the mean time we've published a mocha fork with these patches it in (mocha-papandreou), and Unexpected 5 with support for htmlMessage went out out of beta, so if anyone's interested in getting really nice object diffs that are also colored in the browser, try installing mocha-papandreou@2.1.0-patch2 or above and unexpected 5.2.0 or above.

This refactoring is to first step towards supporting error.htmlMessage.
We restructured the code to have a variable for the error message and
the error stack. This will allow us to use error.htmlMessage instead of
error.message as the message displayed to the user. The next commit will
take advantage of this separation.
If an error has a htmlMessage property it will be inserted verbatim into
the DOM. That gives assertion libraries complete control over the
formatting of the error message shown by the HTML test runner.
@jbnicolai
Copy link

I'm still quite excited about this feature.

// CC @mochajs/mocha

@sunesimonsen
Copy link
Contributor

A lot of stuff has happened since we opened this feature request, Unexpected v7 is the running version and v8 is just around the corner. This feature request is more valid than ever as we attract more and more users and the html reporter is really lacking behind when you are using Unexpected.

We created a documentation site that uses the html serializer from Unexpected to format the samples, that will provide you with more examples of why we want this supported by the Mocha html reporter:
https://unexpectedjs.github.io/

This is such a tiny change that is completely backwards compatible, please don't make us fork the html reporter.

Kind regards
Sune

@jbnicolai
Copy link

@sunesimonsen I completely agree.

Spend some time reviewing the changes and getting a local demo running. All looks great. Rereading the discussion in this thread, it seems no one was opposed to this change - but it was proposed at a bad time (me and @boneskull just started as maintainers and were more than swamped).

Apologies for the long wait.. and I'd be happy to accept your offer to help maintaining this functionality, @papandreou.

Thanks for the hard work!

jbnicolai pushed a commit that referenced this pull request Jun 5, 2015
HTML reporter: Allow the error to contain a formatted HTML error message
@jbnicolai jbnicolai merged commit d71a5c6 into mochajs:master Jun 5, 2015
@sunesimonsen
Copy link
Contributor

@jbnicolai thank you so much, this will be a huge improvement for Unexpected. I know you had a lot going on at the time, no worries at all :-)

@boneskull boneskull removed the status: waiting for author waiting on response from OP - more information needed label Dec 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants