-
Notifications
You must be signed in to change notification settings - Fork 46
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
refactor: use create-jest-runner #37
Conversation
], | ||
jestTestPath: testPath, | ||
}); | ||
const runESLint = ({ testPath, config }) => { |
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 recommend using https://github.com/jest-community/jest-runner-eslint/pull/37/files?w=1 here
@@ -6,7 +6,7 @@ exports[`Works when it has failing tests 1`] = ` | |||
4:3 error 'hello' is not defined no-undef | |||
5:3 error 'bye' is not defined no-undef | |||
✖ 2 problems (2 errors, 0 warnings) | |||
✕ ESLint | |||
✓ ESLint |
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 about this one... @rogeliog thoughts?
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.
This should definitely be a blocker; the check shouldn’t appear if there are any failures.
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.
yup. it was a bug in my port, fixed now 🙂
@@ -6,7 +6,7 @@ exports[`Works when it has failing tests 1`] = ` | |||
4:3 error 'hello' is not defined no-undef | |||
5:3 error 'bye' is not defined no-undef | |||
✖ 2 problems (2 errors, 0 warnings) | |||
✕ ESLint | |||
✓ ESLint |
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.
This should definitely be a blocker; the check shouldn’t appear if there are any failures.
@@ -16,7 +16,7 @@ mocked-stack-trace 3 | if (a === 2) { | |||
6 | } | |||
7 | | |||
2 errors found. | |||
✕ ESLint | |||
✓ ESLint |
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.
Also here
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.
Seems good; it’s a major refactor tho so more eyes are probably good.
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.
This is awesome! Thanks!
This should be a patch, but it’s a risky one, so whoever merges and releases it, please keep an eye out for failed greenkeeper issues popping up. |
I'd say it's a minor (because of added support for run in band), but I agree it comes with some risk. I don't use this runner, could someone who do try to run it on their code base as a sanity check? |
Ah true, minor is good. I’m traveling this week but will try it on Airbnb’s codebase on Monday if it’s not merged by then. |
@ljharb awesome, thanks for that! Yes I think we can wait 😄 |
@ljharb did you get a chance to test it out? |
aah, sorry, no i didn’t yet. I’ll try to make some time to do it next week or the week after! |
My work laptop's died on me, so I'm not sure when I'll be able to test this PR :-/ I don't think that needs to block it, though. |
I tested it in https://github.com/jest-community/eslint-plugin-jest, and it works fine there. @rogeliog thoughts on merging and publishing? |
I just published it under |
Closes #32
Closes #36
Fixes #23
Fixes #20