Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants