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

Switch from jest from ts-jest to swc #2333

Merged
merged 6 commits into from
Sep 6, 2022
Merged

Switch from jest from ts-jest to swc #2333

merged 6 commits into from
Sep 6, 2022

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Sep 5, 2022

Docs: https://swc.rs/ / https://swc.rs/docs/usage/jest

The CI seems to be taking longer and longer to run. This should give us a significant speed boost but there are some caveats.

  1. swc does not type-check the code.
  2. jest.spyOn no longer works out of the box: jest.spyOn does not work when compared ts-jest swc-project/swc#3843
  3. There are 4 webview tests that I cannot get to work. This is due to the above behaviour. I am still looking into it but unsure if I'll be able to find a way round. There is an ugly workaround.

Locally I can run the entire set of Jest tests in ✨ Done in 24.09s.
This previously took: ✨ Done in 46.15s.

CI Now: 61.448s (extension) + 172.465s (webview)
CI Before: 154.413s (extension) + 298.381s (webview)

Note: This is an interesting ticket => swc-project/swc#5205

__esModule: true,
...actualModule
}
})
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] Unfortunately we cannot pull out a helper from this boilerplate as the compiler behaves differently if jest.mock in not written like this in the code.

Logger.log(chunk.toString().replace(/(\r?\n)/g, ''))
)

export const createProcessWithOutput = (options: ProcessOptions) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] moving this removed the need for a spy which was breaking some tests. It is currently only used in a single place so was ok to move.

package.json Outdated Show resolved Hide resolved
setupFiles: ['jest-canvas-mock', '<rootDir>/setup-tests.js'],
testEnvironment: 'node',
testTimeout: 20000
testEnvironment: 'jsdom',
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] Has to be jsdom or everything blows up. Which is fine, we were only doing this to try and get some performance improvements.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we only have to set this here once and not have to add

/**
 * @jest-environment jsdom
 */

for every file, that is very cool.

secondRow,
firstRow,
DragEnterDirection.BOTTOM,
EventCurrentTargetDistances
Copy link
Member Author

Choose a reason for hiding this comment

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

[C] This is gross but I could not find any other way to successfully spy on a single call of getEventCurrentTargetDistances. I had to

  1. give the function its own module
  2. move the import to inside the test file (instead of the util)
  3. mock inside the test file for the usual workaround
  4. pass the module down into the test helper.

I had to do this in 4 places.

@@ -1,6 +1,3 @@
/**
* @jest-environment jsdom
*/
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] No longer required as the whole suite now runs in jsdom.

@mattseddon mattseddon marked this pull request as ready for review September 5, 2022 07:48
@@ -1,5 +1,13 @@
import { Logger } from './logger'

jest.mock('console', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

sendOutput(process)

return process
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we allowed to use this to do things like getting the ast from the project's Python files ?

Copy link
Member Author

Choose a reason for hiding this comment

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

You could definitely try. Take a look at https://github.com/microsoft/vscode-python/tree/main/src/client/languageServer to see what those guys do (outside of the closed source language server)

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is that closed, btw?

Copy link
Member Author

Choose a reason for hiding this comment

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

💰

@@ -0,0 +1,11 @@
import { DragEvent } from 'react'
Copy link
Member Author

Choose a reason for hiding this comment

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

@sroy3 any suggestions for the name of this file?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a good name.

@codeclimate
Copy link

codeclimate bot commented Sep 6, 2022

Code Climate has analyzed commit df875f8 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 92.3% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.9%.

View more on Code Climate.

Copy link
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

setupFiles: ['jest-canvas-mock', '<rootDir>/setup-tests.js'],
testEnvironment: 'node',
testTimeout: 20000
testEnvironment: 'jsdom',
Copy link
Contributor

Choose a reason for hiding this comment

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

If we only have to set this here once and not have to add

/**
 * @jest-environment jsdom
 */

for every file, that is very cool.

@@ -0,0 +1,11 @@
import { DragEvent } from 'react'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a good name.

@mattseddon mattseddon merged commit d384dbf into main Sep 6, 2022
@mattseddon mattseddon deleted the switch-jest-to-swc branch September 6, 2022 20:48
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.

4 participants