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

Provide require.main property #5618

Merged
merged 5 commits into from
Feb 20, 2018
Merged

Provide require.main property #5618

merged 5 commits into from
Feb 20, 2018

Conversation

zamotany
Copy link
Contributor

@zamotany zamotany commented Feb 20, 2018

Closes #2150

Summary

Expose property require.main when testing and set it to module from which the tests are being executed. Currently this property is not set, which is inconsistent with default Node behaviour (Node ref) and the use case was present in #2150.

Test plan

Appropriate unit and integration test cases were added to test if the property is provided in concrete modules and mocks.

Note 1: Module with test suite has to be manually added to _moduleRegistry, since the runtime is created manually instead of using the one from the jest-runtime in which the test are being run.

Note 2: This implementation assumes, that module.parent is set correctly and in order to find main module it traverse parents until module.id and module.parent.id are equal, in which case it would be test suite module.

@codecov-io
Copy link

codecov-io commented Feb 20, 2018

Codecov Report

Merging #5618 into master will increase coverage by 0.83%.
The diff coverage is 30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5618      +/-   ##
==========================================
+ Coverage   60.89%   61.73%   +0.83%     
==========================================
  Files         215      213       -2     
  Lines        7319     7176     -143     
  Branches        4        3       -1     
==========================================
- Hits         4457     4430      -27     
+ Misses       2861     2745     -116     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-runtime/src/index.js 73.83% <30%> (+0.28%) ⬆️
packages/jest-util/src/buffered_console.js 47.16% <0%> (-13.95%) ⬇️
packages/jest-util/src/Console.js 44% <0%> (-12.87%) ⬇️
packages/jest-cli/src/search_source.js 39.28% <0%> (-5.79%) ⬇️
packages/jest-config/src/index.js 25.92% <0%> (-2.65%) ⬇️
packages/jest-haste-map/src/crawlers/watchman.js 89.09% <0%> (-2.58%) ⬇️
packages/jest-runtime/src/script_transformer.js 84.52% <0%> (-0.86%) ⬇️
packages/jest-haste-map/src/index.js 96.3% <0%> (-0.86%) ⬇️
...ckages/jest-cli/src/reporters/coverage_reporter.js 66.14% <0%> (-0.26%) ⬇️
packages/jest-test-typescript-parser/src/index.js 0% <0%> (ø) ⬆️
... and 45 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 9df3035...6e843f3. Read the comment docs.

@thymikee thymikee requested a review from SimenB February 20, 2018 10:57
moduleRequire.requireMock = this.requireMock.bind(this, from.filename);
moduleRequire.resolve = moduleName =>
this._resolveModule(from.filename, moduleName);
Object.defineProperty(
Copy link
Member

Choose a reason for hiding this comment

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

Instead of a traversal, couldn't we add the main module as an instance variable to the runtime instance and then just return it directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would require a to pass some flag in 3rd argument in other packages as well: https://github.com/facebook/jest/blob/master/packages/jest-circus/src/legacy_code_todo_rewrite/jest_adapter.js#L70 and https://github.com/facebook/jest/blob/master/packages/jest-jasmine2/src/index.js#L154
Since we cannot just assume that the first required module is main due to setup files.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, yeah I can live with this then.

@SimenB
Copy link
Member

SimenB commented Feb 20, 2018

Cool! This might help some with #5241 as well, as it rides on some of the same assumptions as main (main's parent should be null)

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.

Changes LGTM.

Changelog? 🙂

@zamotany
Copy link
Contributor Author

@SimenB I updated changelog, but looks like there's some problem with CI itself.

@cpojer cpojer merged commit 4576dd4 into jestjs:master Feb 20, 2018
vzvu3k6k added a commit to vzvu3k6k/gitify that referenced this pull request Oct 21, 2018
vzvu3k6k added a commit to vzvu3k6k/gitify that referenced this pull request Oct 21, 2018
mainModule is blacklisted by jestjs/jest#4955 and require.main
is provided by jestjs/jest#5618, but require.main can't be
accessed on electron.
@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 12, 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.

Add require.main and require.main.filename properties
5 participants