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

remove p-map and fix jasmine promise chain #3880

Merged
merged 2 commits into from
Jun 22, 2017

Conversation

aaronabramov
Copy link
Contributor

this PR fixes two problems:

  1. Fixes the broken promise chain, that caused Jest process to stall with all errors swallowed (node <6)
  2. Require Jasmine from the outer VM scope rather then inner, because modifying global object can break the runner (see Array.prototype[Symbol.iterator] = null stalls the process #3879)

Taking jasmine out seems risky, but all tests seem to be passing

@codecov-io
Copy link

codecov-io commented Jun 22, 2017

Codecov Report

Merging #3880 into master will increase coverage by 0.35%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3880      +/-   ##
=========================================
+ Coverage   57.65%     58%   +0.35%     
=========================================
  Files         195     195              
  Lines        6782    6744      -38     
  Branches        6       6              
=========================================
+ Hits         3910    3912       +2     
+ Misses       2869    2829      -40     
  Partials        3       3
Impacted Files Coverage Δ
packages/jest-jasmine2/src/index.js 0% <0%> (ø) ⬆️
packages/jest-jasmine2/src/jasmine/Env.js 0% <0%> (ø) ⬆️
packages/jest-jasmine2/src/queueRunner.js 100% <100%> (ø) ⬆️
...st-matchers/src/extractExpectedAssertionsErrors.js 13.33% <0%> (ø) ⬆️
packages/jest-jasmine2/src/setup-jest-globals.js 0% <0%> (ø) ⬆️
packages/jest-mock/src/index.js 91.92% <0%> (+0.03%) ⬆️

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 e9b045f...9d6b70e. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Jun 22, 2017

Require Jasmine from the outer VM scope rather then inner, because modifying global object can break the runner (see #3879)

Could something similar be done for #3786?

@thymikee
Copy link
Collaborator

@SimenB I just thought about the same, but that doesn't seem to help.

@aaronabramov
Copy link
Contributor Author

@cpojer said there's more context to this and didn't want me to merge this one yesterday.
I was thinking more about this today, and thought that in the end we should probably have all of our framework code it the outer VM scope and inject some environment specific object from the inner VM scope for every run (if we ever need to do any instanceof operations, or something that requires it to be the exact same constructor object).

e.g.

// in the inner VM scope:
const innerVMScopeContext = new InnerVMScopeContext({
  Symbol,
  Error,
  Function,
  // etc...
});

// return this object to the outer scope where it can be used for whatever it needs to be used for
return innerVMScope;

@@ -92,8 +92,9 @@ function jasmine2(
env.specFilter = spec => testNameRegex.test(spec.getFullName());
}

runtime.requireModule(require.resolve('p-map'));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i thought that would help, but it didn't :)
i ended up removing p-map completely and that helped. I'll remove this line

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay, less deps


let promise = Promise.resolve();

options.queueableFns.forEach(fn => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be replaced with reduce, without creating variable, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so fancy! :)

@aaronabramov aaronabramov changed the title require jasmine from outer VM scope remove p-map and fix jasmine promise chain Jun 22, 2017
Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just wait for CI to finish

@aaronabramov aaronabramov merged commit e7b0a07 into jestjs:master Jun 22, 2017
@aaronabramov aaronabramov deleted the iterator-to-null branch June 22, 2017 20:34
tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
* iterator-to-null test

* move jasmine back to inner scope / don't use iterators
@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 13, 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.

5 participants