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

Support transforming testRunner with preset/transform #8823

Merged
merged 5 commits into from
Nov 15, 2020

Conversation

Mark1626
Copy link
Contributor

Summary

Support transforming testRunner with transform
Part of #8810

cc/ @SimenB

Test plan

Have added a test for transforming testRunner

"testEnvironment": "node",
"transformIgnorePatterns": [
"jest-jasmine2",
"jest-circus"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding jest-circus as well so that when it becomes the default runner the tests won't fail

"jest-jasmine2",
"jest-each",
"jest-circus"
],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These will have to be ignored manually as a relative path to the package is used rather than node_modules.
Seems like the number of ignore paths might increase going forward, one idea I had was to add a pattern like jest-, but that doesn't seem to work in all cases

runtime: Runtime,
testPath: string
): Promise<TestResult> {
return Promise.resolve({
Copy link
Member

Choose a reason for hiding this comment

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

just make the function async rather than Promise.resolve?

Copy link
Member

Choose a reason for hiding this comment

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

we really should have a helper for creating "empty" test results. This is a pain for custom runners (as you can see here)

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.

I'm not sure if we should document that it can be transformed? Since everything should be transformable at some point it might not be worth it I guess

@Mark1626
Copy link
Contributor Author

You mean a single CHANGELOG entry specifying that all everything is transformable?

@SimenB
Copy link
Member

SimenB commented Aug 15, 2019

No, I meant in the documentation. I tmight be what people expect though, so no need. dunno!

@G-Rath
Copy link
Contributor

G-Rath commented Aug 18, 2019

I'm not sure if we should document that it can be transformed? Since everything should be transformable at some point it might not be worth it I guess

Personally I agree w/ this - which is the whole reason why I started this "Transform EVERYTHING!!" spree: it's really unexpected for somethings but not others to be transformed.

You mean a single CHANGELOG entry specifying that all everything is transformable?

I think that's the best way to go about this CHANGELOG wise - otherwise we're all going to be making near-identical CHANGELOG entries.

We could update the same log entry on our PRs, but that'd mean each PR would need to be rebased as the others are merged.


I think what if we opened a new PR right now for just the CHANGELOG entry? that way we could have it prepared & it won't be forgotten, since it'll be there on the list of open PRs.

@Mark1626 Mark1626 force-pushed the feature/transform-testrunner branch from df9a809 to b444350 Compare August 19, 2019 18:08
@Mark1626
Copy link
Contributor Author

@SimenB @G-Rath I've created a util in jest-util for creating an empty test result as suggested.
This will affect the TestResult generated by jest-jasmine2, previously it was missing some of the required fields.

Will be move it to a separate PR, wanted you opinion before that


import {TestResult} from '@jest/test-result';

export default function emptyTestResult(): TestResult {
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 seems to the minimal TestResult template that I could create

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a legacy reason why leaks is mandatory, it looks like an optional field

Copy link
Member

Choose a reason for hiding this comment

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

leaks should be optional, probably just an oversight. It's just a boolean though, right?

@SimenB
Copy link
Member

SimenB commented Aug 20, 2019

I've created a util in jest-util for creating an empty test result as suggested.

Could you add it to @jest/test-result instead? https://github.com/facebook/jest/blob/d523fa84f2a65adf4d18019173dd44600c57bcad/packages/jest-test-result/

@Mark1626
Copy link
Contributor Author

Mark1626 commented Aug 22, 2019

Could you add it to @jest/test-result instead? https://github.com/facebook/jest/blob/d523fa84f2a65adf4d18019173dd44600c57bcad/packages/jest-test-result/

Moving this to a separate PR and reverting it util over here.

@Mark1626
Copy link
Contributor Author

There seems to be a memory leak in the test for node 10.
Not sure if this is coming from this PR

@SimenB
Copy link
Member

SimenB commented Aug 25, 2019

You can rebase for a fix for the leak

@Mark1626 Mark1626 force-pushed the feature/transform-testrunner branch 2 times, most recently from 4fe76fa to 87aaf3f Compare August 29, 2019 18:33
@SimenB SimenB added this to the Jest 26 milestone Apr 28, 2020
@cpojer cpojer removed this from the Jest 26 milestone May 2, 2020
@cpojer cpojer added this to the High priority future milestone May 2, 2020
@SimenB SimenB modified the milestones: High priority future, Jest 27 Oct 23, 2020
@Mark1626
Copy link
Contributor Author

Mark1626 commented Nov 15, 2020

CI is failing due to the yarn lock. The lock file will be modified in this PR as jest-runner will need @jest/transform, I have committed the change in lock as well, can you approve the modification in lock @SimenB

Screenshot 2020-11-15 at 10 39 29 AM

@SimenB
Copy link
Member

SimenB commented Nov 15, 2020

@Mark1626 yeah, that lockfile change looks correct 👍

@Mark1626
Copy link
Contributor Author

Fixing this test now, I'll update once done

/home/runner/work/jest/jest/e2e/__tests__/v8Coverage.test.ts

  ● prints coverage with empty sourcemaps

    expect(received).toBe(expected) // Object.is equality

    Expected: 0
    Received: 1

      34 |   );
      35 | 
    > 36 |   expect(exitCode).toBe(0);
         |                    ^
      37 |   expect(wrap(stdout)).toMatchSnapshot();
      38 | });
      39 | 

      at Object.toBe (e2e/__tests__/v8Coverage.test.ts:36:20)

@@ -6,5 +6,13 @@
*/

module.exports = {
presets: ['@babel/preset-env', '@babel/preset-typescript'],
presets: [
['@babel/preset-typescript'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB I'm totally not sure why thing makes the test pass, any pointers?

Copy link
Member

Choose a reason for hiding this comment

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

doesn't make much sense - this shouldn't be needed at all

Copy link
Member

Choose a reason for hiding this comment

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

it's the same as the others - we either need to not transpile our runner or make sure to transpile it correctly (via adding targets since we don't want weird async function transpilation)

Comment on lines 9 to 12
presets: [
['@babel/preset-typescript'],
[
'@babel/preset-env',
{
targets: {node: 8},
},
],
],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
presets: [
['@babel/preset-typescript'],
[
'@babel/preset-env',
{
targets: {node: 8},
},
],
],
presets: [
['@babel/preset-env', {targets: {node: 'current'}}],
'@babel/preset-typescript',
],

Comment on lines 9 to 12
presets: [
['@babel/preset-typescript'],
[
'@babel/preset-env',
{
targets: {node: 8},
},
],
],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
presets: [
['@babel/preset-typescript'],
[
'@babel/preset-env',
{
targets: {node: 8},
},
],
],
presets: [
['@babel/preset-env', {targets: {node: 'current'}}],
'@babel/preset-typescript',
],

@SimenB
Copy link
Member

SimenB commented Nov 15, 2020

@Mark1626 this should be ready now?

@Mark1626
Copy link
Contributor Author

There is a e2e asyncRegenerator failing after the rebase, looking into it

@Mark1626
Copy link
Contributor Author

This is the failing test I'm trying to fix

e2e/__tests__/asyncRegenerator.test.ts

Screenshot 2020-11-15 at 9 21 41 PM

module.exports = {
  plugins: [
    '@babel/plugin-transform-regenerator',
    '@babel/plugin-transform-runtime',
  ],
  sourceType: 'script',
}

The @babel/plugin-transform-runtime should handle transforming the for of. I'm looking into it, thought I'd ask if this is something trivial I'm missing

"testEnvironment": "node",
"transformIgnorePatterns": [
"jest-jasmine2"
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB This was the reason the last test was failing. Adding it manually to transformIgnorePatterns, again as mentioned in a comment below these ignores are needed only when testing the jest repo, any consumers will not need to add these as these will be within node_modules

@@ -17,6 +17,7 @@
"@jest/console": "^26.6.2",
"@jest/environment": "^26.6.2",
"@jest/test-result": "^26.6.2",
"@jest/transform": "^26.6.2",
Copy link
Member

Choose a reason for hiding this comment

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

Add to references in tsconfig

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's present in tsconfig.json.
Sorry this line should have been part of #8751 , got missed in the rebase I'll be more careful next time

https://github.com/facebook/jest/blob/68d68f5282bb11f93cc24ac41634667469138878/packages/jest-runner/tsconfig.json#L18

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.

👍

@SimenB SimenB merged commit 0a0ac60 into jestjs:master Nov 15, 2020
@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 11, 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