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

Bind to Circus events via an optional event handler on any custom env. #8344

Merged
merged 7 commits into from Apr 19, 2019

Conversation

Projects
None yet
7 participants
@scotthovestadt
Copy link
Contributor

commented Apr 19, 2019

Summary

See #8307 for previous discussion.

This PR simplifies the implementation, exposing less API surface area and providing a clear method of implementation for the end-user.

Test plan

  • All tests pass both with and without JEST_CIRCUS=1.
  • Existing tests fully cover all functionality related to the event handler.
@SimenB

SimenB approved these changes Apr 19, 2019

Copy link
Collaborator

left a comment

Nice and clean! 😀

@SimenB

This comment has been minimized.

Copy link
Collaborator

commented Apr 19, 2019

Maybe add a test that has the new function and just logs every event name or something?

@jeysal

This comment has been minimized.

Copy link
Collaborator

commented Apr 19, 2019

Another thing that came to my mind:
The class / inheritance based test environment abstraction seems non-ideal with this extension. If I want to provide a package that does cool stuff by hooking into jest-circus but allow people to use it with both Node and JSDOM, how do I do that?

export const makeEnvironmentDoCoolStuff = (BaseEnvironment: typeof JestEnvironment) =>
  class CoolStuffEnvironment extends BaseEnvironment {
    handleTestEvent(event, state) {
      super.handleTestEvent(event, state);
      doCoolStuff(event);
    }
  }

It seems like this needs a composition mechanism.

@scotthovestadt

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

@jeysal I agree with you in principle but I tried that first and it's just more prone to errors. I think if we needed to do that, it's not mutually exclusive with also having this automatically available (easy mode) on the environment.

@scotthovestadt

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

@jeysal Also, to answer your core question of "how", I would probably:

  1. Use the new feature I added to allow custom docblocks to be read by the custom env
  2. Define a custom docblock for node vs jsdom
  3. Initialize a new instance of node/jsdom within my custom env and call it's methods manually

Not the greatest but I think it may be the lesser of evils.

@codecov-io

This comment has been minimized.

Copy link

commented Apr 19, 2019

Codecov Report

Merging #8344 into master will decrease coverage by 0.02%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8344      +/-   ##
==========================================
- Coverage   62.19%   62.17%   -0.03%     
==========================================
  Files         266      266              
  Lines       10701    10702       +1     
  Branches     2603     2605       +2     
==========================================
- Hits         6656     6654       -2     
- Misses       3459     3462       +3     
  Partials      586      586
Impacted Files Coverage Δ
packages/jest-circus/src/types.ts 100% <ø> (ø) ⬆️
...circus/src/legacy-code-todo-rewrite/jestAdapter.ts 0% <ø> (ø) ⬆️
packages/jest-circus/src/globalErrorHandlers.ts 17.64% <ø> (ø) ⬆️
...us/src/legacy-code-todo-rewrite/jestAdapterInit.ts 0% <0%> (ø) ⬆️
packages/jest-circus/src/run.ts 0% <0%> (ø) ⬆️
packages/jest-circus/src/eventHandler.ts 7.05% <100%> (ø) ⬆️
packages/jest-circus/src/formatNodeAssertErrors.ts 14.28% <50%> (ø) ⬆️
packages/jest-circus/src/utils.ts 15.38% <61.53%> (ø) ⬆️
packages/jest-circus/src/index.ts 68.05% <80%> (ø) ⬆️
packages/jest-circus/src/state.ts 84.61% <85.71%> (-7.06%) ⬇️
... and 2 more

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 cd415e7...bdaaa99. Read the comment docs.

@scotthovestadt

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

@SimenB test added, will merge on green

@scotthovestadt scotthovestadt merged commit 31e06e8 into facebook:master Apr 19, 2019

11 checks passed

ci/circleci: lint-and-typecheck Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-11 Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
facebook.jest #20190419.5 succeeded
Details
@@ -898,6 +900,12 @@ class CustomEnvironment extends NodeEnvironment {
runScript(script) {
return super.runScript(script);
}
handleTestEvent(event, state) {

This comment has been minimized.

Copy link
@ludeknovy

ludeknovy Apr 23, 2019

This wont work with NodeEnvironment. Current implementation is going to work only with jest-environment.

This comment has been minimized.

Copy link
@SimenB

SimenB Apr 23, 2019

Collaborator

I don't follow? This is jest-environment-node. Or do you mean running circus outside of jest?

This comment has been minimized.

Copy link
@ludeknovy

ludeknovy Apr 23, 2019

Sorry, I misunderstood the jestAdapterInit. Was quicker typing than thinking.

This comment has been minimized.

Copy link
@SimenB

SimenB Apr 23, 2019

Collaborator

Ah, ok. So we're good? :)

This comment has been minimized.

Copy link
@ludeknovy
@SimenB

This comment has been minimized.

Copy link
Collaborator

commented May 25, 2019

@CompuIves this might be something you can use in code sandbox?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.