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

🚀 Feature: Give some control on how diffs are rendered #5088

Open
3 tasks done
forty opened this issue Jan 24, 2024 · 4 comments
Open
3 tasks done

🚀 Feature: Give some control on how diffs are rendered #5088

forty opened this issue Jan 24, 2024 · 4 comments
Labels
status: in discussion Let's talk about it! type: feature enhancement proposal

Comments

@forty
Copy link
Contributor

forty commented Jan 24, 2024

Feature Request Checklist

Overview

I use some custom comparators with chai which allows me to do things such as:

assert.deepEqual({a: 1}, {a: new Interval(0, 10)});

Unfortuantly, when the assertion fails, I don't have any control on how the diff should be rendered, and the diff will expose the internal structure of the Interval class with is not super helpful. I would like to have some control on how the diff is rendered.

Suggested Solution

I thought of a simple solution which works rather well for simple use case, which would be to call toJSON (if available) before object canonicalization (and this way my Interval class could decide a bit how it want to be rendered for the diff).
It feels a fair think to do, and it's already done in the specific case of Buffer class https://github.com/mochajs/mocha/blob/master/lib/utils.js#L218

Alternatives

  • Defining a special function "toMochaDiff" (of something similar) to avoid colliding with other use of toJSON
  • Give user full control over diff rendering by adding hooks to stringifyDiffObjs and generateDiff in base reporter

Additional Info

No response

@forty forty added the type: feature enhancement proposal label Jan 24, 2024
@JoshuaKGoldberg JoshuaKGoldberg added the status: in triage a maintainer should (re-)triage (review) this issue label Feb 9, 2024
@JoshuaKGoldberg
Copy link
Member

This is a really interesting feature idea, thanks for filing 😄. It seems like it'd be ueful!

Though, per #5027, we're trying not to take on any new features that don't have broad user demand. We're more trying to keep Mocha afloat than anything else. So unless other test runners already do this we'd probably avoid it.

Requesting info: what do other test runners do around this, if anything?

@JoshuaKGoldberg JoshuaKGoldberg added status: waiting for author waiting on response from OP - more information needed and removed status: in triage a maintainer should (re-)triage (review) this issue labels Feb 9, 2024
@forty
Copy link
Contributor Author

forty commented Feb 20, 2024

@JoshuaKGoldberg Very good question, thanks for asking :) I had not investigated this at all, so I did some research.

As expected, Jest does a lot, with full battery included style. They have an internal diff module jest-diff, and before diffing, they format object with their own formater lib pretty-format, which allow plenty of customization with plugins and all the bells and whistles. The default behavior involves calling .toJSON, then trying not to if that did not work out well https://github.com/jestjs/jest/blob/e267aff33d105399f2134bad7c8f82285104f3da/packages/jest-matcher-utils/src/index.ts#L108
As you can see, this logic is in their "matcher", which would be a bit akin to saying chai should render their own diff (which in a way also makes sense to me)

Node native test seem also to delegate the diff to the assertion lib. (node assert module produce Errors whose message contains the colored diff directly). It doesn't seem to offer any customisation and doesn't call toJSON or anything

Jasmine seem to also have the diff formating pushed to their matcher (also builtin). They allow customisation of the pretty pretty before diffing https://github.com/jasmine/jasmine/blob/d06dce46141017f9f444caf2b3fe2655c49ad24b/src/core/matchers/DiffBuilder.js#L4C12-L4C26 (and "customObjectFormatters")

My conclusion of all this is that it somewhat more logical for the test runner not to handle diffs, and to push it more to the matcher/assertion level (I imagine assert.deepInclude diff should be computed differently than assert.deepEqual for example and the assertion lib is the one who should know what they mean) though the caveat is that the final rendering of the diff itself is definitely more of the reporter business (for example if you need to render things in HTML rather than in ANSI code - it's not clear to me how the other reporter handle this, maybe with ANSI code to HTML conversion, or maybe by transmitting an intermediate diff representation to the reporter).

For mocha, whose philosophy seem to be more "bring you own xxx" (assertions, mock, etc), I think it would make sense to allow the user to handle their own diff the way they want to. If we want to allow maximum flexibility (and also lowest effort ^^), we could simply have a renderError(error) function in the config, which if not defined, does the current behavior, and if defined, is called an return a string representation of the error, including the diff. As I said, it's not clear to me how HTML should be dealt with. Maybe with a boolean color parameter to renderError to let the rendering know we don't want to have ANSI code.

@JoshuaKGoldberg JoshuaKGoldberg added status: in discussion Let's talk about it! and removed status: waiting for author waiting on response from OP - more information needed labels Feb 20, 2024
@jedwards1211
Copy link

jedwards1211 commented May 16, 2024

@JoshuaKGoldberg I've had two serious problems with diffs in projects that use Sequelize:

If expect(sequelizeModelInstance)... fails, the process prints unable to find module 'pg-native' and crashes before it can format the error diff
If expect(somethingContainingASequelizeTransaction)... fails, the process runs OOM while trying to format the error diff

I'd like to solve this in a more systematic way than just turning off all error diffs or changing code all over my test files.

@jedwards1211
Copy link

@JoshuaKGoldberg Okay, I was able to work around my problems by monkey-patching canonicalize. So it's worth thinking about how to make that whole system officially pluggable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: in discussion Let's talk about it! type: feature enhancement proposal
Projects
None yet
Development

No branches or pull requests

3 participants