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

scotthovestadt
Copy link
Contributor

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.

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.

Nice and clean! 😀

@SimenB
Copy link
Member

SimenB commented Apr 19, 2019

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

@jeysal
Copy link
Contributor

jeysal 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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link

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
Copy link
Contributor Author

@SimenB test added, will merge on green

@scotthovestadt scotthovestadt merged commit 31e06e8 into jestjs:master Apr 19, 2019
@SimenB
Copy link
Member

SimenB commented May 25, 2019

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

@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.

7 participants