Allow custom environments or other setup scripts to bind to Circus events.

[scotthovestadt]

Summary

Currently, it's not possible for custom environments, setup scripts, or even other code inside of Jest to bind to events from Circus. This reduces the value of the new test runner when it could be used for a lot of awesome features that aren't possible today without hacks.

Exposing these events generally has the following use-cases:

• Jest's own reporter should be more granular than just test files. Circus provides the hooks needed to send that information back to the parent process. This is the first step for that feature.
• Some third party code could really use the ability to bind events to specific parts of the test lifecycle. For example, @gribnoysup wrote a great integration for Jest + PollyJS, but it requires manually calling a method in the test context and has to monkey patch it blocks: https://github.com/gribnoysup/setup-polly-jest/blob/master/src/setupJasmine.js#L137

With this change, it's possible to do something like this:

import { addEventHandler, Event } from 'jest-circus'

class MyEnvironment extends NodeEnvironment {
    async setup() {
        addEventHandler(this.onTestEvent.bind(this));
    }

    onTestEvent(event: Event) {
        if (event.name === 'test_start') {
            ...
        }
    }
}

It also works in setupFilesAfterEnv. The key is that the globals have to be setup first by the environment before calling addEventHandler. When writing documentation, I'll make that clear. Unfortunately, it is difficult to detect when it's called too early and warn the user because of the variety of test environments possible. If you can think of a way, let me know.

I'm planning on writing documentation for this but wanted to put this out there first to get any initial feedback on the implementation and the interface for using it. The events themselves and event metadata are fairly Circus-specific so I think it's ideal that any external users realize that and must import circus directly. This will be less awkward when it's the default test runner.

Open to all feedback.

Test plan

• All tests pass, including Circus-specific tests that rely on these events to work properly.
• I've developed an integration against this for something I'm working on, so it definitely works. ;)
• E2E test will be committed before merge, waiting on feedback on the interface.

[scotthovestadt]

@aaronabramov Pretty sure you wrote Circus originally so would appreciate your feedback on what I'm trying to do here.

[SimenB]

This is super exciting stuff! Great idea to add it the the env

[codecov-io]

Codecov Report

Merging #8307 into master will decrease coverage by 0.25%.
The diff coverage is 48.83%.

@@            Coverage Diff             @@
##           master    #8307      +/-   ##
==========================================
- Coverage    62.2%   61.94%   -0.26%     
==========================================
  Files         266      266              
  Lines       10673    10678       +5     
  Branches     2596     2598       +2     
==========================================
- Hits         6639     6615      -24     
- Misses       3445     3477      +32     
+ Partials      589      586       -3

Impacted Files	Coverage Δ
...us/src/legacy-code-todo-rewrite/jestAdapterInit.ts	0% <ø> (ø)	⬆️
packages/jest-circus/src/types.ts	100% <100%> (ø)	⬆️
packages/jest-circus/src/state.ts	37.5% <25%> (-54.17%)	⬇️
packages/jest-circus/src/globals.ts	51.38% <51.38%> (ø)
packages/jest-circus/src/eventHandler.ts	1.17% <0%> (-5.89%)	⬇️
packages/jest-circus/src/utils.ts	13.07% <0%> (-2.31%)	⬇️

---

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 820c2b1...7caef88. Read the comment docs.

[jeysal]

This looks really cool! Can we keep this open in RFCish state for a bit? I'd love to mess around with it as well to see how it covers some use cases

[scotthovestadt]

@jeysal Definitely, please let me know if you have any feedback.

I'd like to get this into the next release within a week-ish but I'll hold off on merging it until necessary.

[gribnoysup]

Thanks for mentioning setup-polly-jest, @scotthovestadt!

Circus was a big question mark for me when working at that jest <> polly integration as I wasn't sure at all how to do it when jasmine is gone 😅Can't wait to start hacking with this branch to prepare for Circus release 😸

[aaronabramov]

that looks amazing! 👍

[SimenB]

This is missing the promised docs, but the implementation looks solid to me! 👍

[scotthovestadt]

I've been using it for a bit now and I feel confident in the interface + no feedback about changing it, will write up docs and then get it merged in shortly!

[SimenB]

@palmerj3 does this help with Spotify's http interceptor?

[jeysal]

I have some time to try this out today :)

[palmerj3]

@SimenB oh man this looks like exactly what I need

[jeysal]

@scotthovestadt I've played around with this and found a use case that this might not cover well. I tried to wrap the test fn in a Zone (basically in add_test replace fn with someApi.wrap(fn)). With this API there is no way to alter the event that circus receives, so I had to work around it and get the already added test using

const test = state.currentDescribeBlock.tests.find(
   ({name}) => name === event.testName,
);

and then alter that TestEntry. This feels hacky and probably does not compose well with other event handlers. Please do tell me if you know a better way to do this.

As an alternative idea for this API, I would suggest using a chain of handlers that can either perform side effects (which the current API seems to be good at) or modify the events (which is not currently possible):

const handler = (event, next) => {
  switch(event.name) {
    case 'add_test':
      console.log(`side effect for ${event.testName}`);
      next({
        ...event,
        fn: someApi.wrap(event.fn),
      })
      break;
    default:
      next(event);
  }
}

The final next() call would invoke the circus logic, which has no further delegate.

Another thing to note is that the handler example has no state argument. I wanted to ask the somewhat separate question if we absolutely need to expose the state itself. This can probably only be answered by looking at a few use cases, but for example formatNodeAssertErrors almost doesn't use the state (except for state.expand, which is really config, not state, and could be done differently). I think often it would be sufficient to just include the necessary information in the event so it can be accessed from there. My experiments also wouldn't need the state if the workaround can be replaced with clean event manipulation.

If we manage to cover the relevant use cases while only exposing events, we can move much faster ™️. Exposing the state means almost any change to its structure will be breaking because of code like the one at the top of this comment. Exposing events and a way to operate on them, on the other hand, means we can e.g. call handlers on the old deprecated event, convert it to the new format, and then call handlers including circus itself on the new event.

Would love to hear thoughts from everyone involved! This makes a quite large and essential piece of previously internal API public, so I think it's not something to be taken lightly :)

[scotthovestadt]

@jeysal Thanks for the feedback!

Public Interface

It's worth noting that the intention of Circus all along was to expose the events and state as public interface so other code could hook into it. It's been a long-standing problem that there's no way to hook into the lifecycle (as evidenced even in this thread by @palmerj3, @gribnoysup, and myself).

Event Mutation

The way Circus works internally is by mutating the event and state objects themselves, and there's no reason other events can't do the same thing. For example, here's the internal code for test_done, which mutates both event and state.

case 'test_done': {
  event.test.duration = getTestDuration(event.test);
  event.test.status = 'done';
  state.currentlyRunningTest = null;
  break;
}

Any change to this is also going to be basically a refactor of the whole Circus runner. I don't think just dropping in a different way of doing things solves any of the core problems. I'm not saying it can't be improved; I'm just saying if we do, it probably belongs in a major version anyway, since it would be a very large refactor.

There are a couple of improvements that can be made incrementally but I didn't make here because I don't have a use-case for them and didn't want to over-engineer:

• Allow event handlers to return Promise<void> and await it. This solves your callback use-case without modifying the interface.
• Allow event handlers to set a priority to be called before/after internal event handlers or each other.

Moving Fast

Yes, this exposes more as a public interface. Much of this is already exposed with the json response we offer, and even more of it will be exposed as jsonLines and more granular test reporting is added. The PRs are related-- we need this to even use it internally outside of Circus files.

If we are truly worried about exposing it to the public quickly, I'm happy to hold off on any documentation. We can use it internally for the use-cases I mentioned and have more time to make any breaking changes needed.

If we do choose to document, these points can be clear in the documentation:

• It could be marked as "EXPERIMENTAL" in documentation with a caveat that breaking changes may be made with only minor version bumps.
• Mutation can be explicitly unsupported.
• Adding new events, new keys to events, or new parts to the state will not be considered a breaking change. We can maintain old keys that aren't used internally with comments to remove in next major.
• Other caveats? Suggestions welcome.

Thanks again for your feedback.

[jeysal]

Thanks for the detailed response @scotthovestadt!

Allow event handlers to return Promise and await it. This solves your callback use-case without modifying the interface.

Could you elaborate on how this solves it? It looks to me like the other bullet point about priorities would solve it by making my handler run before circus' handler, not this one 😄

The way Circus works internally is by mutating the event and state objects themselves, and there's no reason other events can't do the same thing. Any change to this is also going to be basically a refactor of the whole Circus runner.

I'm not suggesting dropping mutation internally. I guess my suggestion is not really about immutability either (the whole next thing is kind of optional, just felt cleaner than mutating the object that the next handler in the chain will also receive). After thinking about it some more I'd say it could be described as exposing "event preprocessors", with the key things that distinguish them from event handlers being

• that they run before the event handlers, which your priority concept addresses, although I think either way we're unfortunately creating kind of a z-index / Babel plugin situation, and
• that they do not have access to circus' state, instead only bending circus to their will by plugging in between globals and event handlers and changing what inputs the circus event handler receives. This is the key to greatly reducing the API surface.

I don't know what exactly you've used custom event handlers for, do you think such an API would be sufficient or are there things which cannot be mapped to event mutations?

[scotthovestadt]

I see your point about mutations. My use-case doesn't involve mutation and you've changed my mind to be explicit about documenting that the mutation use-case isn't supported for now. I think at a minimum we'll need some sort of a priority on the event handler stack to support it -- and as you said, that leads to the zIndex situation, so we can weigh supporting that against any use-cases brought up for it.

Long-term, I think it's going to be important to allowing 3rd party proper hooks into the system without going and adding an explicit hook for them everywhere, but we should discuss it more. One use-case that comes to mind for allowing mutations is being able to remove the custom filter external require we have today and instead you could dynamically hook into the events and automatically skip tests you want to filter out (or just remove them). Someone could create a jest-filter package that did this without even needing custom hooks into the codebase like today. A lot of core code could be moved into packages like this. I think it exposes a huge array of possibilities.

... but we'll defer that for now. The use-case I'm trying to solve is to just hook into various stages of the lifecycle so you can do external things, similar to beforeEach, but with access to more data about what's running and without monkey-patching.

I don't think event pre-handlers are a good solution because most of the data you need for events is generated by the default handlers. Exposing the state is probably necessary because you may need data not in a specific event. The state is just an assembly of parts of the event data, you could just re-create it yourself from all the events... and people will do this if we don't expose state. I think what you actually want is to separate event mutation from state mutation, which makes total sense for mutations but would be a large change to Circus. I agree (tentatively, I'd need to write some code to fully grok it).

To sum up what I've learned from your feedback:

• Document that mutations are currently unsupported.
• Document that things may be added without a breaking change; new events, new keys, etc.
• If we do decide to expose mutation, we may want to refactor Circus internals to provide a cleaner way of doing it that separates event mutation from state mutation.

As-is, what we'll document is that you can synchronously listen to events and access read-only event data and state data. I personally think that this is an acceptable public API surface for a major version and can be broken in a future release if needed.

Lastly, I want to note that jest-circus has 20,894 downloads a week compared to jest-jasmine2 at 4,034,301. I think it's OK to mark everything (edit: everything new) we do with circus as experimental until we ship it as the default to allow us to move fast. Circus is going to be the instrumental in a few key features I want to support:

• Granular support for reporting on individual tests within a file.
• Streaming all results for JSON and to CLI, reducing memory usage from linear to constant.
• Reducing the need for one-off hooks in the codebase, like the custom filter.

[palmerj3]

For my use-case I would ideally need the event handlers to be blocking in some form. Wait for me to resolve a promise, return, etc. Would give time to configure things and clean up before/after a test is run.

But from a reporting standpoint this gets us closer to individual tests instead of a per-suite level we are at now which is great.

[scotthovestadt]

@palmerj3 It's pretty trivial to make the individual event handlers await a Promise if returned, I'll do it in this PR for the initial release. The EventHandler signature will become:

(event: Event, state: State) => void | Promise<void>;

[hong4rc]

I think we don't need to add / after build

[SimenB]

It'll resolve to the index.js path. It could probably be without build as well, and it'll use main

[scotthovestadt]

adding the / wasn't strictly necessary, it's just a result of churn because I changed the import path to something else and then back again

doesn't make a difference either way, it'll resolve to index

[scotthovestadt]

Simplified implementation: #8344

[github-actions]

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.