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

Persist paths in stack trace which have cwd as infix #3244

Merged
merged 1 commit into from Mar 5, 2018

Conversation

outsideris
Copy link
Member

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions.

Description of the Change

As #3093 , when paths in stack trace messages have a current working directory as infix, it should not be replaced. So, I added ( in front of cwd to replace only in a case of the paths started with cwd. I assumed ( is always be there.

Alternate Designs

@ScottFreeCode suggested 'escape-string-regexp' package. However, I don't understand how it fixes this issue.

Why should this be in core?

It is related with display stack trace in mocah.

Benefits

This is an edge case, so the most of users don't experience the issue. Some users like in docker can see correct path in a stack trace.

Possible Drawbacks

If a case of ( is in front of a path can exist, the paths in a stack trace are not replaced with a current working directory.

Applicable issues

#3093

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 90.032% when pulling 364d1d5 on outsideris:issue-3093 into c4ef568 on mochajs:master.

@harrysarson
Copy link
Contributor

How about replacing

if (/\(?.+:\d+:\d+\)?$/.test(line)) {
  line = line.replace(cwd, '');
}

with

if (/\(?.+:\d+:\d+\)?$/.test(line) && line.indexOf(cwd) === 0) {
  line = line.replace(cwd, '');
}

@outsideris
Copy link
Member Author

@harrysarson
The line is like at EventEmitter.<anonymous> (/usr/local/dev/test.js:16:12). So, cwd couldn't be 0 of index.

@harrysarson
Copy link
Contributor

harrysarson commented Feb 15, 2018

Yeah, good point @outsideris.

Replacing based on the presence of '(' would fail for the path: '/prefix/aa(' + process.cwd() + '/file.txt' which is think is a valid file path on Unix

Copy link
Member

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

Hi, Thanks. Please see my question...

@@ -597,7 +597,7 @@ exports.stackTraceFilter = function () {

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

Choose a reason for hiding this comment

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

by using escape-string-regexp, @ScottFreeCode meant do this:

line = line.replace(require('escape-string-regexp')(cwd), '');

Does that not work?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that doesn't.

In this case, escape-string-regexp change nothing on the path. (I didn't check it with escape-string-regexp on Windows)

> var cwd = '/Users/outsideris/github/mocha/';
undefined
> require('escape-string-regexp')(cwd)
'/Users/outsideris/github/mocha/'

So, line = line.replace(require('escape-string-regexp')(cwd), ''); will change at foo (/www/Users/outsideris/github/mocha/foo/index.js:13:226) to at foo (/wwwfoo/index.js:13:226).
It resolves nothing here and results same as before I change it.

@boneskull boneskull added type: bug a defect, confirmed by a maintainer semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Feb 28, 2018
Copy link
Member

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes" type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants