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

New --unified-diff flag makes expected/actual easier to read without colors #862

Closed
wants to merge 10 commits into from

Conversation

rprieto
Copy link
Contributor

@rprieto rprieto commented May 23, 2013

As suggested in this issue: #702

For example, take this test:

var a = { name: 'joe',  age: 30, address: {city: 'new york', country: 'us' }};
var b = { name: 'joel', age: 30, address: {city: 'new york', country: 'usa'}};
a.should.eql(b);

mocha

inline

mocha -C (or looking at text-only logs)

inline no color

mocha --unified-diff

unified diff

mocha --unified-diff -C (or looking at text-only logs)

unified diff no color

It's my first contrib to Mocha, so another pair of eyes would be helpful.
But hopefully this will make troubleshooting tests easier!

@theleoborges
Copy link

+1;
I run my tests from a TextMate bundle -with no colours enabled- and the output is pretty confusing.
This addition would be a great improvement.

@@ -204,6 +205,10 @@ if (~process.argv.indexOf('--colors') ||
Base.useColors = true;
}

// --unified-diff

if (~process.argv.indexOf('--unified-diff')) Base.unifiedDiff = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be if (program.unifiedDiff) <stuff>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I didn't know about that, I'll make the change.

@tj
Copy link
Contributor

tj commented May 23, 2013

I almost think we should just remove the flag and make the + / - default, but ditch the rest of the unified diff output since that's not too useful here

@rprieto
Copy link
Contributor Author

rprieto commented May 23, 2013

Would it be the other way around, with a flag for inline diffs?

Ditching the top/bottom part of the unified diff makes sense, I'll have a look at that later today. We could keep the 3 line context though, it helps knowing where the diff is.

@rprieto
Copy link
Contributor Author

rprieto commented May 24, 2013

A few more fixes:

  • +/- diff is the default
  • removed useless parts of the unified diff
  • new --inline-diff flag to force the old behaviour
  • and I had accidentally broken escaping of invisible characters... should be fixed now

Maybe this could use actual unit tests - for now it's a set of "failing" assertions we need to run & check if the output looks OK.

@bitwiseman
Copy link
Contributor

This is very cool.

Could you could rebase to collapse some of these commits together? Especially, the last six or so.
And yeah, unit tests would be nice too. I've got an error case script in #860. I don't think that will quite cover this, but you could use it as basis.

@rprieto
Copy link
Contributor Author

rprieto commented May 31, 2013

Thanks, all rebased! I'll look at the (lack of) unit tests later this weekend.

@tj
Copy link
Contributor

tj commented May 31, 2013

cool lookin good, apparently still needs a rebase, dont worry about the squash for now I'll pull it down after and tweak a couple things and merge it over as one

@faridnsh
Copy link

Any chance of this getting merged?

@travisjeffery
Copy link
Contributor

💥

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.

6 participants