jest icon indicating copy to clipboard operation
jest copied to clipboard

fix: jest-circus shares events among imports #11483

Open satanTime opened this issue 4 years ago • 26 comments

closes #11483

Summary

The issue is described here: https://github.com/facebook/jest/issues/11483

A public interface to subscribe to events from jest-circus.

Test plan

No changes in UI.

satanTime avatar Jun 05 '21 08:06 satanTime

Hi @satanTime!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

facebook-github-bot avatar Jun 05 '21 08:06 facebook-github-bot

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

facebook-github-bot avatar Jun 05 '21 08:06 facebook-github-bot

Codecov Report

Merging #11529 (fcde463) into main (faef0b4) will decrease coverage by 0.06%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #11529      +/-   ##
==========================================
- Coverage   68.47%   68.41%   -0.07%     
==========================================
  Files         324      324              
  Lines       16967    16972       +5     
  Branches     5060     5062       +2     
==========================================
- Hits        11618    11611       -7     
- Misses       5317     5329      +12     
  Partials       32       32              
Impacted Files Coverage Δ
packages/jest-circus/src/index.ts 70.66% <ø> (ø)
packages/jest-circus/src/state.ts 95.65% <100.00%> (+16.70%) :arrow_up:
packages/jest-circus/src/types.ts 100.00% <100.00%> (ø)
packages/jest-circus/src/eventHandler.ts 0.75% <0.00%> (-9.10%) :arrow_down:
packages/jest-circus/src/formatNodeAssertErrors.ts 9.21% <0.00%> (-2.64%) :arrow_down:
packages/jest-circus/src/utils.ts 11.32% <0.00%> (-0.48%) :arrow_down:

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 faef0b4...fcde463. Read the comment docs.

codecov-commenter avatar Jun 05 '21 08:06 codecov-commenter

HI @SimenB,

the PR has been rebased.

satanTime avatar Sep 04 '21 10:09 satanTime

HI @SimenB,

the PR has been rebased.

satanTime avatar Sep 11 '21 16:09 satanTime

Hi @SimenB,

the PR has been rebased.

satanTime avatar Sep 14 '21 06:09 satanTime

Hi @SimenB,

the PR has been rebased.

satanTime avatar Oct 17 '21 10:10 satanTime

Hi @SimenB, might you explain what the problem with this PR is? Its merge would bring huge relief in life-cycle management.

satanTime avatar Dec 26 '21 10:12 satanTime

Hi @satanTime! I'd just like to say thank you for sticking by this PR without any feedback from me. Unfortunately, I've had little to no combination of time, effort, energy or plain want to work on OSS for months, so this (and other stuff) has just been hanging. The entire 💩storm that is ESM in Node (and specifically its vm API which upstream V8 seems to not care about enabling) completely drained any and all joy I got out of working on OSS in general and Jest specifically.

Hopefully I'll come back to it sooner rather than later, but PRs such as this (where the code itself isn't the hard part, but rather if the underlying issue is something we should support and secondly if the solution here is correct) is not something I'm currently in the correct place to consider

SimenB avatar Feb 01 '22 17:02 SimenB

@SimenB I'm curious, are you the only maintainer of jest now? Is facebook not supporting this project internally anymore?

redonkulus avatar Feb 01 '22 17:02 redonkulus

Nobody at FB (Meta?) has worked on Jest for years at this point, beyond a few PRs here and there like any other open source contributor

SimenB avatar Feb 01 '22 18:02 SimenB

Hi @SimenB, glad to see you alive :) And happy new year!

Open source is huge pain in the hole :) I hope nobody will give up, only the brave!

I've rebased the PR. Please let me know if there is anything I need to change / explain etc :)

satanTime avatar Feb 01 '22 19:02 satanTime

Regarding the issue, I think it's context hell.

Different dependencies require jest-circus in different places and each require creates a different context, that's why require in global scope has its own context and doesn't allow to properly manage EVENT_HANDLERS.

satanTime avatar Feb 01 '22 19:02 satanTime

Nobody at FB (Meta?) has worked on Jest for years at this point, beyond a few PRs here and there like any other open source contributor

I honestly though you were working for Facebook in development of Jest, what a shame :-(

piranna avatar Feb 01 '22 19:02 piranna

I commented it on Twitter: https://twitter.com/mafalda_sfu/status/1488600128680341517

We need to make noise, @meta is still publicing Jest as from them but they are not maintaining anymore, while in fact they should have contracted @SimenB to maintaining it, that's the fair thing to do on this topic.

piranna avatar Feb 01 '22 19:02 piranna

(To avoid potential drama I definetely do not have the energy for, I'll make this preemptive post).

Just to make sure everybody is on the same page - while FB has never paid me (or, as far as I know, any other non-employee) to work on Jest, that doesn't mean there hasn't been opportunities to be paid for the work I and others have put into the project. Specifically the Open Collective has been available, and I've been told multiple times to file expenses against it. However, personally, I've preferred to keep the time I've spent on this project without compensation to avoid feeling compelled to work on it. It might be idealistic, but by not getting paid for the work I've purposefully kept the project from something I've felt obligated to work on. I think by taking the money I'd feel obligated (rightly!) to dedicate time to the project. Which I've never felt comfortable doing. This is a personal choice to keep my independence (and freedom to not interact with the project as much as I probably should the last months), but does not reflect on FB's/Meta's motivations or priorities. My personal choices doesn't mean FB has expected me to work for free, just that I've chosen not to take salary/compensation.

That said I want to be clear there's no "bad guy" here - nobody is forcing me to (or trying to) work on things, and my off hands approach is solely down do lack of time and energy, and not any deeper motivation. Jest has been an open source project for years and continues to be so. Anybody missing any features should continue to feel free to contribute to make those features happen.

(For transparency, FB paid for my and other OSS maintainers trip to London to work physically together on the project back in 2017 and 2018. I can provide more details if people want)

SimenB avatar Feb 01 '22 23:02 SimenB

Hi all, could we keep this thread related to the PR with no drama and a pure focus on solving the issue?

satanTime avatar Feb 02 '22 08:02 satanTime

@SimenB Is Meta using a different testing framework internally, or Jest is just good enough as-is for them?

kibertoad avatar Feb 02 '22 13:02 kibertoad

Hi @SimenB, the PR has been rebased. What stops you from including in it the next major release?

The issue is context:

currently it is like that:

const a = () => {
  const context = {};

  return context;
};

a() === a(); // false

when it should be

global.context = global.context || {};

const a = () => {
  return global.context;
};

a() === a(); // true and everybody is happy

satanTime avatar Feb 12 '22 11:02 satanTime

Hi @satanTime,

Seeing the time needed for this to be merged, maybe you could publish a gist to use along with https://www.npmjs.com/package/patch-package, installed via a postinstall script ? Just wondering if this is doable.... Of course, hotpatching jest isn't a viable long term solution, but it might at least be a POC users will use, and eventually give confidence to maintainers to (finally) merge this PR.

I'm having the feeling that this package, used by a lot of people, would gain by redefining contributors permissions & include more people, so that you/we could have more people to talk to to bring evolutions in.

Cheers

GerkinDev avatar Feb 15 '22 09:02 GerkinDev

Hi @GerkinDev, it's a good idea, thanks for the hint. I just need to find time to implement it.

satanTime avatar Feb 19 '22 13:02 satanTime

I wanted to give an update, in order to address the maintenance concerns mentioned in this thread, Jest has been transferred to a foundation. Hopefully this will help! https://engineering.fb.com/2022/05/11/open-source/jest-openjs-foundation/

vjeux avatar May 11 '22 17:05 vjeux

Hi @vjeux,

not sure if that's changed anything, in the article it says "Jest joining the OpenJS foundation does not change how Jest is developed or released".

@SimenB, is there a way to get someone from your team to take a look at this PR? Without this change, and knowing about the issue in hooks https://github.com/facebook/jest/issues/12678, jest becomes a framework, which is very hard to support in 3rd-party libraries.

satanTime avatar Jan 08 '23 11:01 satanTime

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: satanTime (3b5066071a3d02277ebe8c3db529e6770f40da87, d9cda507c07db32a0a38dc112013ac1042058e32)

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
Latest commit 3b5066071a3d02277ebe8c3db529e6770f40da87
Latest deploy log https://app.netlify.com/sites/jestjs/deploys/665b3a94d77171000867f2e3
Deploy Preview https://deploy-preview-11529--jestjs.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Apr 30 '23 15:04 netlify[bot]

Hi @SimenB, happy Wednesday!

Is there an opportunity to get your attention on this issue again? The fix is here for more than 2 years.

satanTime avatar Nov 15 '23 17:11 satanTime