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

the same context for addEventHandler from jest-circus/src/state.ts #11483

Open
satanTime opened this issue May 29, 2021 · 18 comments · May be fixed by #11529
Open

the same context for addEventHandler from jest-circus/src/state.ts #11483

satanTime opened this issue May 29, 2021 · 18 comments · May be fixed by #11529

Comments

@satanTime
Copy link

satanTime commented May 29, 2021

🐛 Bug Report

The problem is that I need to distinguish scopes on every beforeAll and beforeEach call.
However, adding a listener on addEventHandler doesn't bring the desired effect, because the listener is added to a different context and hasn't been ever called.

With jasmine it was quite easy:

jasmine.getEnv().addReporter(reporterStack);

And would be great to have the same behavior in jest-circus.

To Reproduce

This is the test and desired code: https://github.com/satanTime/jest-circus-hooks/blob/master/src/app/app.component.spec.ts#L7-L21

Steps to reproduce the behavior:

> jest -w 1 --config jest.js

 FAIL  src/app/app.component.spec.ts
  main block
    ✕ uses listener (2 ms)
    sub block
      ✓ simulates positive (3 ms)
  • edit jest.ts and uncomment testRunner: 'jest-jasmine2',
  • npm run test
  • no failure

This is the place I want to add my callback to.

https://github.com/facebook/jest/blob/master/packages/jest-circus/src/state.ts#L14

Expected behavior

addEventHandler register a callback properly and the callback is triggered on events.

For example, when I change the source code in node_modules to:

const eventHandlers = window.eventHandlers || [_eventHandler.default, _formatNodeAssertErrors.default];
window.eventHandlers = eventHandlers;

it works correctly.

Proposed PR with a fix

The PR is here #11529

envinfo

  System:
    OS: macOS 11.4
    CPU: (16) x64 Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
  Binaries:
    Node: 12.22.1 - /opt/local/bin/node
    Yarn: 1.22.10 - /opt/local/bin/yarn
    npm: 6.14.13 - /opt/local/bin/npm
  npmPackages:
    jest: 27.0.3 => 27.0.3
@klizter
Copy link

klizter commented Jun 3, 2021

Bump!

@satanTime
Copy link
Author

Hi there, I'll prepare a min demo today / next days.

@satanTime
Copy link
Author

Added a min repo with steps.

satanTime added a commit to satanTime/jest that referenced this issue Jun 5, 2021
satanTime added a commit to satanTime/jest that referenced this issue Jun 5, 2021
satanTime added a commit to satanTime/jest that referenced this issue Jun 5, 2021
@satanTime
Copy link
Author

I've created a PR with a fix.

satanTime added a commit to satanTime/jest that referenced this issue Jun 5, 2021
satanTime added a commit to satanTime/jest that referenced this issue Jun 5, 2021
satanTime added a commit to satanTime/jest that referenced this issue Jun 5, 2021
@klizter
Copy link

klizter commented Jun 7, 2021

Great work @satanTime

@satanTime
Copy link
Author

satanTime commented Jun 16, 2021

Hi @SimenB,

I hope this finds you well.

There are 9 upvotes already, could you take a look at the issue and its fix when you have time?

Thank you in advance!

satanTime added a commit to satanTime/jest that referenced this issue Jun 23, 2021
@satanTime
Copy link
Author

Hi @SimenB, could you take a look at the issue and the related PR?

Thank you in advance!

satanTime added a commit to satanTime/jest that referenced this issue Jun 30, 2021
@satanTime
Copy link
Author

Hi @SimenB,

there are 40 upvotes and a proposed fix.
Might you take a look when you have time or forward me to a person who could take care about code review?

Thank you in advance.

@satanTime
Copy link
Author

Hi @SimenB,

I hope this finds you well.

However, it is a pity to see how you treat your community.
There are 60 upvotes within less than 2 months.
The issue is one of the most upvoted...

...and zero feedback from your side.

How should open source encourage devs to participate in it, when an easy fix stumbles over а disregard from the repo maintainers?

@sigveio
Copy link
Contributor

sigveio commented Jul 29, 2021

To be fair - the reason it's one of the most upvoted is probably because people are being prompted by ng-mocks to go and do so. So the upvote count doesn't necessarily reflect the real impact of this issue on the community as a whole, or how urgent it is to fix compared to other things.

I understand that it's frustrating to put time and effort into proposing a fix without getting response in a timely manner. But I feel the tone in your last comment is unnecessarily sharp/negative. Warranted or not, I don't think it will help at all.

Simen (and I'm sure others) are doing a tremendous effort to bring us awesome tooling here, and I bet there are a lot of different things demanding their attention every single day. We should try to remain respectful to each other and not add to the burden they are already carrying.

It's also worth keeping in mind that many have had a demanding year and a half with the pandemic. And that it's currently vacation time in much of Europe. So expect extra delays.

Meanwhile... perhaps there are some qualified individuals amongst those 60 who could take the time to review the PR and not just upvote? It might help move it along when someone with access to merge have time to look at it.

✌️ @ ❤️

@satanTime
Copy link
Author

Hi @sigveio, you are welcome to review the PR.

@szakharchenko
Copy link

I can only speak for myself, but I stumbled upon this PR when searching for a way to add test event handlers, and it looked much like how I would have done it. For a reasonably sized package, I would have just forked it privately, exposed the functionality and never bothered to search in the first place, and ultimately would never know people taking voting advice from NPM packages was a thing, so maybe it was all for the best;).

satanTime added a commit to satanTime/jest that referenced this issue Sep 4, 2021
satanTime added a commit to satanTime/jest that referenced this issue Sep 9, 2021
satanTime added a commit to satanTime/jest that referenced this issue Sep 11, 2021
satanTime added a commit to satanTime/jest that referenced this issue Sep 14, 2021
satanTime added a commit to satanTime/jest that referenced this issue Sep 18, 2021
satanTime added a commit to satanTime/jest that referenced this issue Feb 1, 2022
satanTime added a commit to satanTime/jest that referenced this issue Feb 12, 2022
satanTime added a commit to satanTime/jest that referenced this issue Feb 19, 2022
@jaytonic
Copy link

Any chance the PR can be approved? Not sure to understand what is the blocking point since last summer?

@satanTime
Copy link
Author

The main point is that maintainers don't understand the fix and don't have time to understand it.

Although, I thought, it is an easy one.

@mircoservices
Copy link

mircoservices commented Apr 26, 2022

For anyone interested here a workaround using patch-package and [email protected]:

Add file patches/jest-circus 28.1.3.patch to your workspace:

diff --git a/node_modules/jest-circus/build/index.d.ts b/node_modules/jest-circus/build/index.d.ts
index adab032..a1ed485 100644
--- a/node_modules/jest-circus/build/index.d.ts
    b/node_modules/jest-circus/build/index.d.ts
@@ -113,6  113,10 @@ export declare const setState: (state: Circus.State) => Circus.State;
 
 export declare type State = Circus.State;
 
 export declare const addEventHandler: (handler: Circus.EventHandler) => void;
 
 export declare const removeEventHandler: (handler: Circus.EventHandler) => void;
 
 declare const test_2: Global.It;
 export {test_2 as test};
 
diff --git a/node_modules/jest-circus/build/index.js b/node_modules/jest-circus/build/index.js
index 1c848bf..7e9c6d4 100644
--- a/node_modules/jest-circus/build/index.js
    b/node_modules/jest-circus/build/index.js
@@ -35,6  35,18 @@ Object.defineProperty(exports, 'setState', {
     return _state.setState;
   }
 });
 Object.defineProperty(exports, 'addEventHandler', {
   enumerable: true,
   get: function () {
     return _state.addEventHandler;
   }
 });
 Object.defineProperty(exports, 'removeEventHandler', {
   enumerable: true,
   get: function () {
     return _state.removeEventHandler;
   }
 });
 exports.test = void 0;
 
 var _jestEach = require('jest-each');
diff --git a/node_modules/jest-circus/build/state.js b/node_modules/jest-circus/build/state.js
index 528b774..f194111 100644
--- a/node_modules/jest-circus/build/state.js
    b/node_modules/jest-circus/build/state.js
@@ -32,7  32,7 @@ function _interopRequireDefault(obj) {
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
  */
-const eventHandlers = [_eventHandler.default, _formatNodeAssertErrors.default];
 global[_types.EVENT_HANDLERS] = global[_types.EVENT_HANDLERS] || [_eventHandler.default, _formatNodeAssertErrors.default];
 const ROOT_DESCRIBE_BLOCK_NAME = 'ROOT_DESCRIBE_BLOCK';
 exports.ROOT_DESCRIBE_BLOCK_NAME = ROOT_DESCRIBE_BLOCK_NAME;
 
@@ -74,7  74,7 @@ const setState = state => (global[_types.STATE_SYM] = state);
 exports.setState = setState;
 
 const dispatch = async event => {
-  for (const handler of eventHandlers) {
   for (const handler of global[_types.EVENT_HANDLERS]) {
     await handler(event, getState());
   }
 };
@@ -82,7  82,7 @@ const dispatch = async event => {
 exports.dispatch = dispatch;
 
 const dispatchSync = event => {
-  for (const handler of eventHandlers) {
   for (const handler of global[_types.EVENT_HANDLERS]) {
     handler(event, getState());
   }
 };
@@ -90,7  90,14 @@ const dispatchSync = event => {
 exports.dispatchSync = dispatchSync;
 
 const addEventHandler = handler => {
-  eventHandlers.push(handler);
   global[_types.EVENT_HANDLERS].push(handler);
 };
 
 exports.addEventHandler = addEventHandler;
 
 const removeEventHandler = handler => {
   const i = global[_types.EVENT_HANDLERS].indexOf(handler);
   global[_types.EVENT_HANDLERS].splice(i, 1);
 };
 
 exports.removeEventHandler = removeEventHandler;
diff --git a/node_modules/jest-circus/build/types.js b/node_modules/jest-circus/build/types.js
index 70116de..5f552fb 100644
--- a/node_modules/jest-circus/build/types.js
    b/node_modules/jest-circus/build/types.js
@@ -25,3  25,6 @@ const TEST_TIMEOUT_SYMBOL = Symbol.for('TEST_TIMEOUT_SYMBOL');
 exports.TEST_TIMEOUT_SYMBOL = TEST_TIMEOUT_SYMBOL;
 const LOG_ERRORS_BEFORE_RETRY = Symbol.for('LOG_ERRORS_BEFORE_RETRY');
 exports.LOG_ERRORS_BEFORE_RETRY = LOG_ERRORS_BEFORE_RETRY;
 
 const EVENT_HANDLERS = Symbol.for('EVENT_HANDLERS');
 exports.EVENT_HANDLERS = EVENT_HANDLERS;

satanTime added a commit to satanTime/jest that referenced this issue May 8, 2022
satanTime added a commit to satanTime/jest that referenced this issue Sep 11, 2022
satanTime added a commit to satanTime/jest that referenced this issue Jan 8, 2023
satanTime added a commit to satanTime/jest that referenced this issue Apr 15, 2023
satanTime added a commit to satanTime/jest that referenced this issue Apr 16, 2023
satanTime added a commit to satanTime/jest that referenced this issue Apr 30, 2023
satanTime added a commit to satanTime/jest that referenced this issue Nov 7, 2023
satanTime added a commit to satanTime/jest that referenced this issue Nov 7, 2023
satanTime added a commit to satanTime/jest that referenced this issue Nov 7, 2023
satanTime added a commit to satanTime/jest that referenced this issue Nov 7, 2023
satanTime added a commit to satanTime/jest that referenced this issue Nov 7, 2023
satanTime added a commit to satanTime/jest that referenced this issue Nov 7, 2023
satanTime added a commit to satanTime/jest that referenced this issue Nov 7, 2023
satanTime added a commit to satanTime/jest that referenced this issue Nov 7, 2023
satanTime added a commit to satanTime/jest that referenced this issue Nov 7, 2023
satanTime added a commit to satanTime/jest that referenced this issue Nov 7, 2023
satanTime added a commit to satanTime/jest that referenced this issue Nov 15, 2023
satanTime added a commit to satanTime/jest that referenced this issue Nov 15, 2023
Copy link

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Feb 22, 2024
@Proper-Job
Copy link

We're still having to patch jest circus like @mircoba suggested: #11483 (comment)

@github-actions github-actions bot removed the Stale label Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants