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

Make file URLs in error output more user friendly. #969

Merged
merged 2 commits into from
Feb 25, 2019

Conversation

edhager
Copy link
Contributor

@edhager edhager commented Feb 14, 2019

Fixes #952.

This PR makes URLs contained in errors printed to the browser console more user friendly by doing two things:

  1. Spaces are added around the URL so the browsers do not include the ">" as part of the URL:

    at regular suite  < http://localhost:9000/_tests/tests/unit/lib/reporters/Html.js:87:21 >
    

    With this change, clinking on the link does the following:

    • Chrome opens the file at the designated line number in the "Sources" tab in the development tools.
    • Safari, opens the file at the designated line number in the "Resources" tab in the development tools.
    • Firefox tries to open the file in a new tab. Without the next change, this request fails.
    • IE and Edge do nothing because they do not turn the URL into a hyperlink.
  2. To support the request that Firefox makes when the URL is clicked in the console, a URL filter was added to the server code to remove the line and column number references so the request is handled properly.

@edhager edhager requested a review from jason0x43 February 14, 2019 19:44
@codecov-io
Copy link

codecov-io commented Feb 14, 2019

Codecov Report

Merging #969 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #969      +/-   ##
=========================================
+ Coverage   92.68%   92.7%   +0.01%     
=========================================
  Files          58      59       +1     
  Lines        4595    4605      +10     
  Branches      998     999       +1     
=========================================
+ Hits         4259    4269      +10     
  Misses        336     336
Impacted Files Coverage Δ
src/lib/Server.ts 78.61% <100%> (+0.27%) ⬆️
src/lib/middleware/filterUrl.ts 100% <100%> (ø)
src/lib/common/ErrorFormatter.ts 95.28% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46c785a...7e641d9. Read the comment docs.

@jason0x43
Copy link
Member

jason0x43 commented Feb 22, 2019

Finally got a chance to look at this. In my own tests, it seems to work as described -- great!

I agree, though, that the spaces seem less than ideal. What if we used a format more like Chrome's for the locations, like

at regular suite @ http://localhost:9000/_tests/tests/unit/lib/reporters/Html.js:87:21

The <> were originally used so the output would look similar to browser stack listings. However, browsers seem to have adopted simpler formats at this point.

@edhager
Copy link
Contributor Author

edhager commented Feb 25, 2019

@jason0x43 I updated the code to use @ instead of < >.

@jason0x43 jason0x43 merged commit 5cef6a5 into theintern:master Feb 25, 2019
jason0x43 pushed a commit that referenced this pull request Mar 4, 2019
* Make file URLs in error output more user friendly.

* Use "@" in error messages rather than wrap source file in "<>".
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