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

Global functions: add pretty error for syntax error. #6901

Closed
wants to merge 2 commits into from

Conversation

binygal
Copy link
Contributor

@binygal binygal commented Aug 28, 2018

Provide explanation that configuration transform entry is not working
on the global functions.

Summary

Right now Jest is not running transformation on setup and teardown global functions and this is confusing for the users, this PR prettify the error message on syntax error to note that transform is not supported in global functions. (#5164)

Test plan

Check that the new copy is actually printed to the screen as can be seen in the provided screenshots.
(2 tests added, using an invalid globalSetup and globalTeardown, and matching the process stderr against a regex with the "not supported" copy)
screen shot 2018-08-28 at 7 17 58
screen shot 2018-08-28 at 7 18 41

Provide explanation that configuration transform entry is not working
on the global functions.
@binygal binygal force-pushed the global-setup-transform branch from 46174f8 to 37942c8 Compare August 28, 2018 04:27
@codecov-io
Copy link

codecov-io commented Aug 28, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@66560c3). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #6901   +/-   ##
=========================================
  Coverage          ?   66.92%           
=========================================
  Files             ?      250           
  Lines             ?    10383           
  Branches          ?        4           
=========================================
  Hits              ?     6949           
  Misses            ?     3433           
  Partials          ?        1
Impacted Files Coverage Δ
packages/jest-cli/src/runJest.js 64.21% <0%> (ø)

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 66560c3...600ffe4. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Aug 30, 2018

This loses the if SyntaxError part of the error, as well as the file name and line. We should keep that info as well

@SimenB
Copy link
Member

SimenB commented Aug 30, 2018

Played around locally:
image

Not sure I'm happy with it...

@binygal
Copy link
Contributor Author

binygal commented Sep 3, 2018

@SimenB Do you think the approach of better message on error is wrong?

@SimenB
Copy link
Member

SimenB commented Oct 22, 2018

@binygal sorry about not responding earlier.

I do think providing a better message is the right thing to do, but your current approach removes the syntax error itself making it harder for the user to track down their error.

My local playing around (that I don't think I have available anywhere anymore) basically re-added the information that was lost, and added a code frame. If you can accomplish something like that'd, I'd love to merge it 🙂

@binygal binygal closed this Jan 20, 2019
@binygal binygal deleted the global-setup-transform branch January 20, 2019 15:24
@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants