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

[Part 1] Individual Test Case Report - Support custom messages #10293

Merged
merged 24 commits into from
Jul 21, 2020

Conversation

kunal-kushwaha
Copy link
Contributor

@kunal-kushwaha kunal-kushwaha commented Jul 20, 2020

Summary ⚡

PART 1 of 2

Aim: Support custom messages for jest worker
Ref: #9662

This PR part of an effort to break down PR #10227 , so that we can eventually have #6616

Test plan ⚡

An extension from #9662

Closes #9662

Acknowledgements

@sauravhiremath for his super awesome contribution 🚀 🚀
@rogeliog for laying down the foundation for this PR. 🚀
@SimenB and @jevakallio for their time and reviews 👏

@kunal-kushwaha kunal-kushwaha changed the title Feature/custom messages [Part 1] Individual Test Case Report - Support custom messages Jul 20, 2020
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay!

method: string,
...args: Array<any>
): PromiseWithCustomMessage<unknown> {
const customMessageListeners: Set<OnCustomMessage> = new Set();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const customMessageListeners: Set<OnCustomMessage> = new Set();
const customMessageListeners = new Set<OnCustomMessage>();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okayy, I think I missed these changes which I committed at a later stage in the previous PR; when I split it. Sorry for the repeat suggestions 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries!

const addCustomMessageListener = (listener: OnCustomMessage) => {
customMessageListeners.add(listener);
return () => {
// Check if the following check is required
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need to loop, just call the delete


const onEnd: OnEnd = (error: Error | null, result: unknown) => {
customMessageListeners.clear();
return error ? reject(error) : resolve(result);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep if-else

WorkerPoolInterface,
WorkerPoolOptions,
} from './types';
import _messageParent from './workers/messageParent';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import _messageParent from './workers/messageParent';
export {default as messageParent} from './workers/messageParent';

@@ -146,3 +148,6 @@ export default class JestWorker {
return this._workerPool.end();
}
}

export type {PromiseWithCustomMessage};
export const messageParent = _messageParent;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export const messageParent = _messageParent;


const isWorkerThread = () => {
try {
const {isMainThread, parentPort} = require('worker_threads');
Copy link
Member

@SimenB SimenB Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do import on this - we only support node 10 and up these days

@sauravhiremath sauravhiremath force-pushed the feature/custom-messages branch from f8fbd6b to 45492fa Compare July 21, 2020 06:53
const onEnd: OnEnd = (error: Error | null, result: unknown) => {
customMessageListeners.clear();
if (error) {
return reject(error);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return does nothing

parentProcess: NodeJS.Process = process,
): void => {
if (isWorkerThread()) {
const {parentPort} = require('worker_threads');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to require here

import {isMainThread, parentPort} from 'worker_threads';
import {PARENT_MESSAGE_CUSTOM} from '../types';

const isWorkerThread = () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can just be const isWorkerThread = !isMainThread && parentPort. If we want it to be lazy, at least ditch the try-catch

@SimenB
Copy link
Member

SimenB commented Jul 21, 2020

Huh, seems we need the try catch for worker_threads looking at CI failure. Might be behind experimental flag on node 10

@sauravhiremath
Copy link
Contributor

Huh, seems we need the try catch for worker_threads looking at CI failure. Might be behind experimental flag on node 10

Yeah for node v10 LTS it requires the expiremental flag

@sauravhiremath
Copy link
Contributor

Huh, seems we need the try catch for worker_threads looking at CI failure. Might be behind experimental flag on node 10

@SimenB Do I open a PR for adding this?

@SimenB
Copy link
Member

SimenB commented Jul 21, 2020

Nah, we should just keep the try-catch here. I'd forgotten it's flagged in v10

@SimenB SimenB merged commit 3150a81 into jestjs:master Jul 21, 2020
@SimenB
Copy link
Member

SimenB commented Jul 21, 2020

🎉🎉🎉

@SimenB
Copy link
Member

SimenB commented Jul 21, 2020

Btw, could you send a PR updating the readme of jest-worker with this new API?

@sauravhiremath sauravhiremath deleted the feature/custom-messages branch August 3, 2020 05:32
@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 11, 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.