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

Only replace absolute -> relative paths for stack lines #2145

Merged
merged 2 commits into from
Mar 21, 2016

Conversation

Turbo87
Copy link
Contributor

@Turbo87 Turbo87 commented Mar 7, 2016

This PR resolves #2126.

the stack can include the error message which might contain paths that should not be replaced
@Turbo87
Copy link
Contributor Author

Turbo87 commented Mar 9, 2016

@danielstjules any comments on this?

@danielstjules
Copy link
Contributor

It looks good, but can you post the output of the code snippet from #2126 (comment) with your branch? Will take a look later tonight.

@Turbo87
Copy link
Contributor Author

Turbo87 commented Mar 10, 2016

@danielstjules it will change to

  1) foo bar:
     Error: this is a test in /Users/tbn/Code/tmp/mocha-stack-bug/foo/bar/test.js and it fails
       at Context.<anonymous> (test/test.js:5:11)

in other words: the paths in the stacktrace are still simplified, but any paths in the original error message remain the same

@Turbo87
Copy link
Contributor Author

Turbo87 commented Mar 17, 2016

@danielstjules ping

@danielstjules
Copy link
Contributor

Sorry for the delay! Confirmed that the following works as described after the PR:

describe('foo', function() {
  it('bar', function() {
    throw new Error('this is a test in ' + process.cwd() + '/foo/bar/test.js and it fails');
  });
});
----------------
Before PR
----------------
  1) foo bar:
     this is a test in /Users/danielstjules/git/mocha/foo/bar/test.js and it fails
  Error: this is a test in foo/bar/test.js and it fails

----------------
After PR
----------------
  1) foo bar:
     Error: this is a test in /Users/danielstjules/git/mocha/foo/bar/test.js and it fails

Edit: Fixed output

@@ -733,7 +733,11 @@ exports.stackTraceFilter = function() {
}

// Clean up cwd(absolute)
list.push(line.replace(cwd, ''));
if (/\(?.+:\d+:\d+\)?$/.test(line)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the capture group for? Could this be simplified, e.g.

if (line.match(/:\d+:\d+$/)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's actually not a capture group, those are escaped parenthesis. your suggestion would only work for stack lines without parenthesis around the file+position info, but according to the test/utils.js both seem to exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, didn't notice the escape characters :)

@danielstjules
Copy link
Contributor

Other than my comment on the regexp, looks good :)

danielstjules added a commit that referenced this pull request Mar 21, 2016
Only replace absolute -> relative paths for stack lines
@danielstjules danielstjules merged commit 4301caa into mochajs:master Mar 21, 2016
@Turbo87 Turbo87 deleted the fix-stack-filter branch March 21, 2016 08:17
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.

Stack trace filtering causing duplicate error messages
2 participants