Move coverage worker into jest-worker #4596
Conversation
|
I would also like to get more information on the It looks like this was added to avoid a possible race condition if the parent dies; if that's the case, I think the best alternative here is to add this |
|
This is not the coverage worker (in jest-cli), this is jest-runner… :O |
|
The test failure seems ok (something that was |
| rawModuleMap: watcher.isWatchMode() | ||
| ? test.context.moduleMap.getRawModuleMap() | ||
| : null, | ||
| : undefined, |
There was a problem hiding this comment.
this is causing the single test failure
| }, | ||
| TEST_WORKER_PATH, | ||
| ); | ||
| // $FlowFixMe: "default" property will be exposed. |
There was a problem hiding this comment.
It would be cool if default (as long as __esModule: true) would be the actual default export instead of attached to the default key (so you could just do worker(stuff)).
There was a problem hiding this comment.
I have been thinking about this, and I'm happy to see feedback on that direction. The reason of the default comes from the fact that a class instance is returned from new. The change here would be to make constructor return a method, but then instanceof JestWorker wouldn't work.
I will think more about it and see what's the best way of fixing the issue; but generally speaking, I agree with your comment.
| } catch (error) { | ||
| callback(formatError(error)); | ||
| } catch (err) { | ||
| throw formatError(err); |
There was a problem hiding this comment.
any particular reason formatError return something not an actual error? I realise that is not new in this PR, it just struck me as odd
|
Are you not going to acknowledge that this PR is converting a different piece of code than it says in the title? :P I'm hesitant to merge this because it runs user tests which may fail, and currently there is no retry or throw/fail/exit strategy in jest-worker like we discussed last week. I'm more happy to merge this for the coverage worker, because that one runs code from a more controlled environment. |
|
Yes, it's true; that's not the right piece of code we were initially intended to transform; however the PR itself is still valid; so I will keep it parked while we fix the retry issue on |
|
Yea, not saying anything is wrong with this PR, just adding context :) |
|
See #4825 |
|
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. |
4 participants
This PR moves the coverage workers into the new
jest-workermodule; which also allows to use typed arguments and results on both sides. Tests were updated to match the new APIs.