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

fix(reporter): Fix #2930 causing error stack not to be parsed correctly #2932

Merged
merged 1 commit into from
Mar 1, 2018
Merged

Conversation

doberkofler
Copy link
Contributor

Fixed a problem when config.urlRoot is set to '/' causing the error stack not to be parsed correctly and therefore source maps are not used.

@doberkofler
Copy link
Contributor Author

@lusarz You have approved my changes, but there are a lot of unit tests failing in my push request and I guess i got it wrong. Did you already review the changes and problems?

@lusarz
Copy link
Contributor

lusarz commented Feb 22, 2018

@doberkofler
True, I shouldn't approve (although it doesn't matter because I'm not team member). I looked a little more closely to this issue, and I created minimal reproduction:
https://github.com/lusarz/karma-hello-world/tree/karma-sourcemap-issue-reproduction

Seems like this pull request solve problem, don't know why these unit tests fails - I'll try to figure it out.

@doberkofler
Copy link
Contributor Author

@lusarz I noticed the failing unit tests but because most of them seem to be related to the the changes in 2.0, I did not feel as if I should just change them to “my” new behavior.
For me it was a simple regression, but I do not fully understand the background for the changes in 2.0.

lib/reporter.js Outdated
@@ -9,7 +9,7 @@ var baseReporterDecoratorFactory = require('./reporters/base').decoratorFactory

var createErrorFormatter = function (config, emitter, SourceMapConsumer) {
var basePath = config.basePath
var urlRoot = config.urlRoot || ''
var urlRoot = config.urlRoot !== '/' ? config.urlRoot : ''
Copy link
Contributor

Choose a reason for hiding this comment

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

@doberkofler
What about:

var urlRoot = config.urlRoot === '/' ? '' : (config.urlRoot || '')

@doberkofler
Copy link
Contributor Author

@lusarz Looks good to me. Just committed it to my fork to do some testing.

@lusarz
Copy link
Contributor

lusarz commented Feb 22, 2018

@doberkofler
I see on travis that unit tests are ok now. Travis throw one error:

The command "if [ "$VALIDATE_COMMIT_MSG" == "true" ]; then ./scripts/validate-commit-msg.sh $TRAVIS_COMMIT; fi" failed and exited with 1 during .

Which seems to be related with format of commit message.

@doberkofler
Copy link
Contributor Author

doberkofler commented Feb 22, 2018

@lusarz I have changed the commit message and the Travis build now passed.
The AppVeyor build still failed and I have no clue on what this is all about?

@lusarz
Copy link
Contributor

lusarz commented Feb 22, 2018

@doberkofler
Yeah, I noticed same issue on one of my pull request. It's not related with content of this pr (issue with useragent dependency). I was investigating a bit around it, and reported a few issues/points:
#2913 (comment)
3rd-Eden/useragent#120

Seems that no authorized person has time to look at this :(

@lusarz
Copy link
Contributor

lusarz commented Feb 23, 2018

Since this pr is merged, appveyor build should works.

Copy link
Member

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

can you add a test for this please

@doberkofler
Copy link
Contributor Author

@dignifiedquire I've added a unit test

@dignifiedquire dignifiedquire merged commit ac4e1a9 into karma-runner:master Mar 1, 2018
@dignifiedquire
Copy link
Member

Thank you :octocat:

@lusarz
Copy link
Contributor

lusarz commented Mar 22, 2018

@dignifiedquire Can I please for release into npm with this pull request?

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.

3 participants