-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
feat: transform file paths into hyperlinks #8980
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8980 +/- ##
==========================================
+ Coverage 63.9% 63.92% +0.01%
==========================================
Files 277 277
Lines 11652 11655 +3
Branches 2860 2860
==========================================
+ Hits 7446 7450 +4
Misses 3576 3576
+ Partials 630 629 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Looks like there's no downside to adding this either, given the fallback.
Can we add a test for this?
I wonder, does it make sense to put this hyperlink there? It only works in iTerm, and there I can cmd+click the relative path anyway (at least on my environment) 🤔 |
Fair point. Now I'm not sure about the absolute paths, as they occupy a lot of space (we could make them less intrusive by dimming the color). And I'm not a fan of adding a flag for that. Maybe we could do this when there's a lot of demand for such feature? |
Agreed, it should be a pretty easy change of adjusting/conditionally removing the fallback if it's needed. Now regarding the test, do you have any ideas for it? I mean testing if it renders a hyperlink when supported would be kinda testing the library itself wouldn't it? |
I think in this case both would be fine:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if there's a test we can write that can reliably guard against a regression - happy to be proven wrong though 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the fallback is key for this. So that this is just a progressive enhancement for some terminals, but keep the current behavior for other terminals
added a test, @jeysal let me know if you have any suggestions |
Nice, that's about one of the options I was thinking of. Only thing that could be added is to mock a return value for terminal-link and check that it is in the output |
this should do it, what do you think @jeysal ? |
Sorry, I can assure you I haven't forgot about it, just been very busy. I'll take a look ASAP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright I took another look and left a few nits on the test :)
packages/jest-reporters/src/__tests__/get_result_header.test.js
Outdated
Show resolved
Hide resolved
const call = terminalLink.mock.calls[0]; | ||
|
||
expect(terminalLink).toHaveBeenCalled(); | ||
expect(call[0]).toBe(formatTestPath(globalConfig, testResult.testFilePath)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer if we hardcode the expected call argument values here without using formatTestPath
/testResult
, it kinda reimplements the production code this way and is not easy on the eye.
Also I think toHaveBeenCalledWith
simplifies this a lot (4 lines to 1)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried hardcoding the formatTestPath
result by copy-pasting it from the console, but it contains the parts which are coloured by the chalk library and it seems to get lost when copied, any idea how could I keep it if we still want to go hardcode it?
btw. is reimplementing the production code in tests a bad practice? if so, what's exactly wrong with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could mock chalk like some other tests do so it just becomes something like <green>text</green>
. But that's probably too complicated. Maybe just expect.stringContaining('the test path')
?
What I mean re reimplementing: The reason we write tests is to run the code with specific example data that reproduces a case simple enough to wrap our heads around. If we could manage the complexity of all cases in all the code at once, we wouldn't need tests but could just look at the production code and see whether it's correct ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, thanks for the explanation @jeysal ! I've hardcoded the expected values, let me know if that's what you had in mind :)
packages/jest-reporters/src/__tests__/get_result_header.test.js
Outdated
Show resolved
Hide resolved
great, thorough review, thanks for that! |
47b6168
to
bd09213
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @lekterable! These are the kind of UX details that make Jest great! :)
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Closes #8937
Summary
Straightforward change, file paths will now become hyperlinks in terminals that support it. In the ones that don't they will stay the same. I was thinking about displaying the paths somehow (flag??) as they are still clickable and quite useful in the Terminal.app even though they are not exactly hyperlinks.
I've used commonJS modules to import the lib as we don't have
esModuleInterop
flag in TS config, can you see any reasons for not using it?Test plan
Hyperlink in ITerm2:
![Zrzut ekranu 2019-09-25 o 19 33 57](https://user-images.githubusercontent.com/23037261/65625105-76714280-dfcb-11e9-9468-84907bd2edd4.png)
Hyperlink in ITerm2 when hovered with CMD pressed:
![Zrzut ekranu 2019-09-25 o 19 15 11](https://user-images.githubusercontent.com/23037261/65625056-5e99be80-dfcb-11e9-9f89-082129c9b4fa.png)
Hyperlink in Terminal.app without a fallback:
![Zrzut ekranu 2019-09-25 o 19 16 14](https://user-images.githubusercontent.com/23037261/65625181-9ef93c80-dfcb-11e9-8720-05bbf062970d.png)
^ It's ugly, but useful which makes me wonder if it would be worth it to add some kind of flag for it.