-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Conversation
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. |
interesting was just looking @ some of the diff code because it handles dates poorly. |
@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. |
35c1580
to
64dfc0b
Compare
@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 :) |
Don't know when/if @jbnicolai will have time to look @ this. So I have a couple things.
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. |
@boneskull Thanks for the feedback!
The CSS3 stuff introduced by 5e695c7 was copied from the
No browsers support it, it's an extra property we propose adding to We've also thought about other options, such as making
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. |
@papandreou Would PR #1357 have any impact on this PR? |
@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 :) |
5e695c7
to
0c18ad0
Compare
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 |
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.
I'm still quite excited about this feature. // CC @mochajs/mocha |
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: This is such a tiny change that is completely backwards compatible, please don't make us fork the html reporter. Kind regards |
@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! |
HTML reporter: Allow the error to contain a formatted HTML error message
@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 :-) |
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
anderr.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. binaryBuffer
orUint8Array
) 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
anderr.expected
and including an ansi-colored diff inerr.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):