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

Configurable max diff size #4799

Merged
merged 2 commits into from Feb 15, 2022
Merged

Conversation

norla
Copy link
Contributor

@norla norla commented Dec 15, 2021

Description of the Change

Fixes #4767

Mocha truncates large diffs before displaying a readable unified/inline comparison.
This means that they are not displayed at all for larger strings in many cases, which causes confusion.
This PR makes the limit for when diffs should be truncated configurable (via "--reporter-option maxDiffSize=number"), so that projects that have large diffs can still get the outout they expect.

The default limit for generating diffs is also increased from 2048 to 8192 character, so that fewer people will run into this situation to begin with.

Also fixes bug causing the message about text being truncated to disappear.

Alternate Designs

Why should this be in core?

Fix for things already in core :)

Benefits

Configurable limit ensures that each project can configure the cropping of strings before shoving diff according to their own needs.

Possible Drawbacks

One more config flag to consider/document etc.

Applicable issues

#4767

@nopeless
Copy link

Personally never ran into this issue, but I can see myself running into this in the future 👍

@sz-piotr
Copy link

I have run into this multiple times, would really appreciate this getting more attention.

@juergba
Copy link
Member

juergba commented Feb 5, 2022

@norla thank you for this PR.

I'm not convinced of appending an additional option for this parameter.
Nevertheless we already have a reporter specific option reporter-option which could be enhanced for this case. We already use it for writing test results to a file.

Usage:

  • CLI: --reporter-option diffSize=4048
  • config files: 'reporter-option': ['diffSize=4048']

@norla
Copy link
Contributor Author

norla commented Feb 8, 2022

@juergba makes sense, didn't know about reporter options until now :)
The PR is updated accordingly, I named the option "maxDiffSize".

Another thing I just thought of:
The default value 2048 seems very conservative, maybe it can be increased to something like 8192 or 16384 instead?
I think that would prevent a lot of people from having to fiddle with "maxDiffSize" in the first place.

@juergba
Copy link
Member

juergba commented Feb 8, 2022

@norla thank you, I will have a look this week.

@coveralls
Copy link

coveralls commented Feb 8, 2022

Coverage Status

Coverage increased (+0.01%) to 94.444% when pulling 40bac95 on norla:configurable-max-diff-size into 509938d on mochajs:master.

docs/index.md Outdated Show resolved Hide resolved
lib/mocha.js Outdated Show resolved Hide resolved
lib/reporters/base.js Outdated Show resolved Hide resolved
lib/reporters/base.js Show resolved Hide resolved
@juergba juergba added area: reporters involving a specific reporter semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Feb 11, 2022
@juergba juergba added this to the next milestone Feb 11, 2022
Copy link

@drGeoBN drGeoBN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@juergba
Copy link
Member

juergba commented Feb 11, 2022

@norla please don't push commit by commit, our resources on the CI servers e.g. netlify are limited.
Test locally, then push occasionally.

@juergba
Copy link
Member

juergba commented Feb 11, 2022

@norla thank you so far. I will do some testing tomorrow.
Evtl. I can find a way to simplify this Number.isNaN check a little bit.

You could update this issue description (reporter-option / maxDiffSize, 2048=> 8192, fix message) and squash your commits.
Then lets see what the test coverage is telling.

Edit: ... and rebase to master.

@juergba juergba added the run-browser-test run browser tests on forked PR if code is safe label Feb 12, 2022
@github-actions github-actions bot removed the run-browser-test run browser tests on forked PR if code is safe label Feb 12, 2022
lib/reporters/base.js Outdated Show resolved Hide resolved
lib/reporters/base.js Outdated Show resolved Hide resolved
lib/reporters/base.js Outdated Show resolved Hide resolved
@norla
Copy link
Contributor Author

norla commented Feb 12, 2022 via email

@norla
Copy link
Contributor Author

norla commented Feb 13, 2022

I've fixed your comments. A bit unsure about the extra test but hopefully it should be enough. Let me know otherwise.
Still need to rebase and squash, though. Tomorrow :)

@juergba
Copy link
Member

juergba commented Feb 14, 2022

@norla thank you very much! Please rebase/squash and mention the --reporter-option maxDiffsize=number information in your issue description. I'm planning to release this fix next weekend.

@juergba juergba added the run-browser-test run browser tests on forked PR if code is safe label Feb 15, 2022
@github-actions github-actions bot removed the run-browser-test run browser tests on forked PR if code is safe label Feb 15, 2022
@juergba juergba merged commit 11c4560 into mochajs:master Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: reporters involving a specific reporter semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Diffs over 2048 characters are truncated even when there is no performance problem
6 participants