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

[WIP][Proposal] Report progress of individual test cases #9001

Closed
wants to merge 13 commits into from

Conversation

rogeliog
Copy link
Contributor

@rogeliog rogeliog commented Oct 2, 2019

This is a big PR and it has a lot of stuff that is still pending, but I think it is a good place to start the conversation of how we can solve #6616.

Before

before

After

report-after

I would love to get feedback on what would be good APIs for all of this.

cc: @SimenB @thymikee @cpojer @aaronabramov

This PR basically does the following things:

  1. Allow jest-worker to send custom messages back to the main process, so that we can notify when a test case is done.
  2. Builds on top of @SimenB's approach of using an event emitter for this in the TestScheduler side. Report progress of individual test cases #6616 (comment)
  3. Implements the API that @rickhanlonii suggested Report progress of individual test cases #6616 (comment)
  4. Update the reporter and Status to handle test case result.

Concerns and more

  1. We should find a good way of allowing test runners to use the event emitter API (AKA "I support notifying when a test case is done"). Right now I have it based on this field testRunnter.__PRIVATE_UNSTABLE_API_supportsEventEmmiters__ as a temporary solution.
  2. I still don't fully like my proposed API for jest-worker to handle custom messages, which is to extend the Promise interface to have a .onCustomMessage(cb). @mjesun I'm sure you might a better solution than me.
  3. I am worried about performance, given that with this approach we are serializing and deserializing an AssertionResult for each test case in each test suite. If this does have a perf hit, we could be smarter on this and potentially throttle and batch them.
  4. I would love to get this to work with the VerboseReporter(see each test case name and error as they go) but with the current implementation it is really hard.
  5. I still don't like "sendMessageToJest" option that I added to jest-circus. It feels that all almost shouldn't belong there, but I didn't have a better place for it.

Assuming that we resolve the issues mentioned above, these items are still pending.

  • tests
  • more tests
  • docs

@rogeliog
Copy link
Contributor Author

@SimenB @thymikee @cpojer @rickhanlonii @scotthovestadt I was wondering if you all had some thoughts on this 🙃

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.

Hey @rogeliog, sorry about the slow response! Swamped at work for the last month...

I know @scotthovestadt has worked on a streaming report thing over in #8242 - would this PR help implement that? Would be great to coordinate a bit, as they solve two sides of the same issue (I think?)

Would it make sense to land the parts of this that makes sense on its own (if any)? E.g. moving to event based architecture for reporters makes sense to me regardless of this feature landing. Same with sending custom messages with jest-worker. I might be wrong though, and they make more sense together

(I just skimmed the code, I'd need to sit down and properly read this through to review properly, which I currently don't have the time for...)

@@ -52,6 +54,7 @@ export const initialize = ({
localRequire: (path: Config.Path) => any;
testPath: Config.Path;
parentProcess: Process;
sendMessageToJest?: Function;
Copy link
Member

Choose a reason for hiding this comment

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

type it properly?

const valuesForCurrentTestCases = getValuesCurrentTestCases(
options ? options.currentTestCases : [],
);
// console.log(aggregatedQuickStats.numPassingTests);
Copy link
Member

Choose a reason for hiding this comment

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

remove?

(testsTodo ? chalk.bold.magenta(`${testsTodo} todo`) + ', ' : '') +
(testsPassed ? chalk.bold.green(`${testsPassed} passed`) + ', ' : '') +
`${testsTotal} total`;
(testsFailed
Copy link
Member

Choose a reason for hiding this comment

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

woah, hard to read. thoughts on a .map().filter().join() instead? might not help, dunno. if not, maybe a helper of some sort?

@slangberg
Copy link

What is the progress of the PR?

@SimenB
Copy link
Member

SimenB commented Feb 24, 2022

A version of this landed in #10227, thanks for getting the ball rolling @rogeliog! ❤️

@SimenB SimenB closed this Feb 24, 2022
@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 Mar 27, 2022
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