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

fix: jest-circus shares events among imports #11483 #11529

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

satanTime
Copy link

@satanTime satanTime commented Jun 5, 2021

closes #11483

Summary

The issue is described here: #11483

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

Test plan

No changes in UI.

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented Jun 5, 2021

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 cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented Jun 5, 2021

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

@codecov-commenter
Copy link

@codecov-commenter codecov-commenter commented Jun 5, 2021

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%) ⬆️
packages/jest-circus/src/types.ts 100.00% <100.00%> (ø)
packages/jest-circus/src/eventHandler.ts 0.75% <0.00%> (-9.10%) ⬇️
packages/jest-circus/src/formatNodeAssertErrors.ts 9.21% <0.00%> (-2.64%) ⬇️
packages/jest-circus/src/utils.ts 11.32% <0.00%> (-0.48%) ⬇️

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.

Copy link

@fredericojesus fredericojesus left a comment

Tks for this PR. Looking forward for seeing it released. I don't see any issue with the code, like the approach.

Copy link

@szakharchenko szakharchenko left a comment

Exposing testing events in the API makes so much sense that I basically assumed it was there when I started using Jest, only to find out that it wasn't. The proposed changes seem in line with what's already in there. One may argue that this is a feature, not a fix, and adjust the CHANGELOG.md entry, but other than that, things look fine.

krusche
krusche approved these changes Aug 7, 2021
@satanTime
Copy link
Author

@satanTime satanTime commented Sep 4, 2021

HI @SimenB,

the PR has been rebased.

@satanTime satanTime force-pushed the feature/11483 branch 2 times, most recently from a37105c to ee436cb Compare Sep 11, 2021
@satanTime
Copy link
Author

@satanTime satanTime commented Sep 11, 2021

HI @SimenB,

the PR has been rebased.

@satanTime
Copy link
Author

@satanTime satanTime commented Sep 14, 2021

Hi @SimenB,

the PR has been rebased.

@satanTime
Copy link
Author

@satanTime satanTime commented Oct 17, 2021

Hi @SimenB,

the PR has been rebased.

@satanTime satanTime force-pushed the feature/11483 branch 2 times, most recently from 902c7c2 to 1206b5b Compare Oct 31, 2021
@satanTime satanTime force-pushed the feature/11483 branch 2 times, most recently from 526902a to e99a588 Compare Dec 11, 2021
@satanTime
Copy link
Author

@satanTime satanTime commented Dec 26, 2021

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

@SimenB
Copy link
Collaborator

@SimenB SimenB commented Feb 1, 2022

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 SimenB mentioned this pull request Feb 1, 2022
21 tasks
@redonkulus
Copy link

@redonkulus redonkulus commented Feb 1, 2022

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

@SimenB
Copy link
Collaborator

@SimenB SimenB commented Feb 1, 2022

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

@satanTime
Copy link
Author

@satanTime satanTime commented Feb 1, 2022

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

@satanTime satanTime commented Feb 1, 2022

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.

@piranna
Copy link
Contributor

@piranna piranna commented Feb 1, 2022

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

@piranna piranna commented Feb 1, 2022

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.

@SimenB
Copy link
Collaborator

@SimenB SimenB commented Feb 1, 2022

(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)

@satanTime
Copy link
Author

@satanTime satanTime commented Feb 2, 2022

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

@kibertoad

This comment was marked as off-topic.

@satanTime
Copy link
Author

@satanTime satanTime commented Feb 12, 2022

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

@GerkinDev
Copy link

@GerkinDev GerkinDev commented Feb 15, 2022

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

@satanTime
Copy link
Author

@satanTime satanTime commented Feb 19, 2022

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

@vjeux
Copy link
Contributor

@vjeux vjeux commented May 11, 2022

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/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.