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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"pseudoterminal",
"rwlock",
"sonarjs",
"spyable",
"stackframe",
"subdir",
"submodules",
Expand Down
6 changes: 4 additions & 2 deletions extension/jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ module.exports = {
coverageDirectory: 'coverage/jest',
coveragePathIgnorePatterns: ['<rootDir>/src/test/'],
coverageReporters: ['json'],
preset: 'ts-jest',
testEnvironment: 'node',
testPathIgnorePatterns: [
'<rootDir>/src/test/',
'<rootDir>/dist/',
'<rootDir>/.vscode-test',
'<rootDir>/.wdio-vscode-service'
]
],
transform: {
'^.+\\.(t|j)sx?$': ['@swc/jest']
}
}
3 changes: 2 additions & 1 deletion extension/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -1607,6 +1607,8 @@
"uuid": "8.3.2"
},
"devDependencies": {
"@swc/core": "1.2.247",
"@swc/jest": "0.2.22",
"@types/chai": "4.3.3",
"@types/chai-as-promised": "7.1.5",
"@types/copy-webpack-plugin": "10.1.0",
Expand Down Expand Up @@ -1644,7 +1646,6 @@
"shx": "0.3.4",
"sinon": "14.0.0",
"sinon-chai": "3.7.0",
"ts-jest": "29.0.0-next.1",
"ts-loader": "9.3.1",
"vsce": "2.11.0",
"vscode-uri": "3.0.3",
Expand Down
8 changes: 8 additions & 0 deletions extension/src/common/logger.test.ts
Original file line number Diff line number Diff line change
@@ -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.

const actualModule = jest.requireActual('console')
return {
__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.


describe('Logger', () => {
describe('error', () => {
it('should be able to write an error to the console', () => {
Expand Down
14 changes: 0 additions & 14 deletions extension/src/processExecution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { Readable } from 'stream'
import { Event, EventEmitter } from 'vscode'
import { Disposable } from '@hediet/std/disposable'
import execa from 'execa'
import { Logger } from './common/logger'

interface RunningProcess extends ChildProcess {
all?: Readable
Expand Down Expand Up @@ -59,16 +58,3 @@ export const executeProcess = async (
const { stdout } = await createProcess(options)
return stdout
}

const sendOutput = (process: Process) =>
process.all?.on('data', chunk =>
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.

const process = createProcess(options)

sendOutput(process)

return process
}
27 changes: 14 additions & 13 deletions extension/src/python/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,44 +1,45 @@
import { join } from 'path'
import { setupVenv } from '.'
import * as ProcessExecution from '../processExecution'
import { Process, createProcess } from '../processExecution'
import { getProcessPlatform } from '../env'

jest.mock('../env')
jest.mock('../processExecution')

const mockedGetProcessPlatform = jest.mocked(getProcessPlatform)
const createProcessSpy = jest.spyOn(ProcessExecution, 'createProcess')
const mockedCreateProcess = jest.mocked(createProcess)
const mockedProcess = {
all: { on: jest.fn() }
} as unknown as ProcessExecution.Process
} as unknown as Process

beforeEach(() => {
jest.resetAllMocks()
})

describe('setupVenv', () => {
it('should create the correct python processes on sane operating systems', async () => {
createProcessSpy.mockResolvedValue(mockedProcess)
mockedCreateProcess.mockResolvedValue(mockedProcess)
mockedGetProcessPlatform.mockReturnValue('freebsd')

const envDir = '.env'
const cwd = __dirname

await setupVenv(__dirname, envDir, 'dvc')

expect(createProcessSpy).toHaveBeenCalledTimes(3)
expect(createProcessSpy).toHaveBeenCalledWith({
expect(mockedCreateProcess).toHaveBeenCalledTimes(3)
expect(mockedCreateProcess).toHaveBeenCalledWith({
args: ['-m', 'venv', envDir],
cwd,
executable: 'python3'
})

expect(createProcessSpy).toHaveBeenCalledWith({
expect(mockedCreateProcess).toHaveBeenCalledWith({
args: ['-m', 'pip', 'install', '--upgrade', 'pip', 'wheel'],
cwd,
executable: join(cwd, envDir, 'bin', 'python')
})

expect(createProcessSpy).toHaveBeenCalledWith({
expect(mockedCreateProcess).toHaveBeenCalledWith({
args: ['-m', 'pip', 'install', '--upgrade', 'dvc'],
cwd,
executable: join(cwd, envDir, 'bin', 'python')
Expand All @@ -47,27 +48,27 @@ describe('setupVenv', () => {

it('should create the correct python processes on windows', async () => {
mockedGetProcessPlatform.mockReturnValue('win32')
createProcessSpy.mockResolvedValue(mockedProcess)
mockedCreateProcess.mockResolvedValue(mockedProcess)

const envDir = '.env'
const cwd = __dirname

await setupVenv(__dirname, envDir, '-r', 'requirements.txt')

expect(createProcessSpy).toHaveBeenCalledTimes(3)
expect(createProcessSpy).toHaveBeenCalledWith({
expect(mockedCreateProcess).toHaveBeenCalledTimes(3)
expect(mockedCreateProcess).toHaveBeenCalledWith({
args: ['-m', 'venv', envDir],
cwd,
executable: 'python'
})

expect(createProcessSpy).toHaveBeenCalledWith({
expect(mockedCreateProcess).toHaveBeenCalledWith({
args: ['-m', 'pip', 'install', '--upgrade', 'pip', 'wheel'],
cwd,
executable: join(cwd, envDir, 'Scripts', 'python.exe')
})

expect(createProcessSpy).toHaveBeenCalledWith({
expect(mockedCreateProcess).toHaveBeenCalledWith({
args: ['-m', 'pip', 'install', '--upgrade', '-r', 'requirements.txt'],
cwd,
executable: join(cwd, envDir, 'Scripts', 'python.exe')
Expand Down
16 changes: 15 additions & 1 deletion extension/src/python/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,21 @@ import { join } from 'path'
import { getVenvBinPath } from './path'
import { getProcessPlatform } from '../env'
import { exists } from '../fileSystem'
import { createProcessWithOutput } from '../processExecution'
import { Logger } from '../common/logger'
import { createProcess, Process, ProcessOptions } from '../processExecution'

const sendOutput = (process: Process) =>
process.all?.on('data', chunk =>
Logger.log(chunk.toString().replace(/(\r?\n)/g, ''))
)

export const createProcessWithOutput = (options: ProcessOptions) => {
const process = createProcess(options)

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.

💰


const installPackages = (cwd: string, pythonBin: string, ...args: string[]) => {
return createProcessWithOutput({
Expand Down
8 changes: 5 additions & 3 deletions webview/jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@ module.exports = {
moduleNameMapper: {
'\\.(scss|css|less)$': 'identity-obj-proxy'
},
preset: 'ts-jest',
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.

testTimeout: 20000,
transform: {
'^.+\\.(t|j)sx?$': ['@swc/jest']
}
}
5 changes: 3 additions & 2 deletions webview/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,17 @@
"vega-lite": "^5.2.0"
},
"devDependencies": {
"@storybook/testing-library": "0.0.13",
"@storybook/addon-essentials": "6.5.10",
"@storybook/addon-interactions": "6.5.10",
"@storybook/addons": "6.5.10",
"@storybook/builder-webpack5": "6.5.10",
"@storybook/manager-webpack5": "6.5.10",
"@storybook/preset-scss": "1.0.3",
"@storybook/react": "6.5.10",
"@storybook/testing-library": "0.0.13",
"@svgr/cli": "6.3.1",
"@swc/core": "1.2.247",
"@swc/jest": "0.2.22",
"@testing-library/jest-dom": "5.16.5",
"@testing-library/react": "13.3.0",
"@types/classnames": "2.3.1",
Expand Down Expand Up @@ -77,7 +79,6 @@
"storybook-addon-designs": "6.3.1",
"storybook-addon-themes": "6.1.0",
"style-loader": "3.3.1",
"ts-jest": "29.0.0-next.1",
"ts-loader": "9.3.1",
"webpack": "5.74.0",
"webpack-cli": "4.10.0",
Expand Down
11 changes: 8 additions & 3 deletions webview/setup-tests.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
const StyleUtils = require('./src/util/styles')
jest.mock('./src/util/styles', () => {
const actualModule = jest.requireActual('./src/util/styles')
return {
__esModule: true,
...actualModule,
getThemeValue: jest.fn().mockImplementation(() => '#ffffff')
}
})

// eslint-disable-next-line no-global-assign
window = {
Expand All @@ -23,5 +30,3 @@ const intersectionObserverMock = jest.fn().mockImplementation(() => {
}
})
global.IntersectionObserver = intersectionObserverMock

StyleUtils.getThemeValue = jest.fn().mockImplementation(() => '#ffffff')
3 changes: 0 additions & 3 deletions webview/src/experiments/components/App.test.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
/**
* @jest-environment jsdom
*/
/* eslint jest/expect-expect: ["error", { "assertFunctionNames": ["expect", "expectHeaders"] }] */
import {
cleanup,
Expand Down
11 changes: 8 additions & 3 deletions webview/src/experiments/components/table/Table.test.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
/**
* @jest-environment jsdom
*/
/* eslint jest/expect-expect: ["error", { "assertFunctionNames": ["expect", "expectHeaders"] }] */
import '@testing-library/jest-dom/extend-expect'
import { configureStore } from '@reduxjs/toolkit'
Expand Down Expand Up @@ -34,6 +31,14 @@ import { experimentsReducers } from '../../store'
import { customQueries } from '../../../test/queries'

jest.mock('../../../shared/api')
jest.mock('../../hooks/useColumnOrder', () => {
const actualModule = jest.requireActual('../../hooks/useColumnOrder')
return {
__esModule: true,
...actualModule
}
})

const { postMessage } = vsCodeApi
const mockedPostMessage = jest.mocked(postMessage)

Expand Down
22 changes: 18 additions & 4 deletions webview/src/plots/components/App.test.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
/**
* @jest-environment jsdom
*/
import { join } from 'dvc/src/test/util/path'
import { configureStore } from '@reduxjs/toolkit'
import React, { ReactElement } from 'react'
Expand Down Expand Up @@ -50,6 +47,17 @@ import {
} from '../../test/dragDrop'
import { DragEnterDirection } from '../../shared/components/dragDrop/util'
import { clearSelection, createWindowTextSelection } from '../../test/selection'
import * as EventCurrentTargetDistances from '../../shared/components/dragDrop/currentTarget'

jest.mock('../../shared/components/dragDrop/currentTarget', () => {
const actualModule = jest.requireActual(
'../../shared/components/dragDrop/currentTarget'
)
return {
__esModule: true,
...actualModule
}
})

jest.mock('../../shared/api')

Expand Down Expand Up @@ -1122,7 +1130,13 @@ describe('App', () => {
})

const plots = screen.getAllByTestId(/^plot_/)
dragEnter(plots[0], plots[1].id, DragEnterDirection.RIGHT)

dragEnter(
plots[0],
plots[1].id,
DragEnterDirection.RIGHT,
EventCurrentTargetDistances
)

const plotsWithDropTarget = screen.getAllByTestId(/^plot_/)

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
/**
* @jest-environment jsdom
*/
import { configureStore } from '@reduxjs/toolkit'
import '@testing-library/jest-dom/extend-expect'
import {
Expand Down Expand Up @@ -30,10 +27,20 @@ import { DragEnterDirection } from '../../../shared/components/dragDrop/util'
import { plotsReducers } from '../../store'
import { webviewInitialState } from '../webviewSlice'
import { getThemeValue, hexToRGB, ThemeProperty } from '../../../util/styles'
import * as EventCurrentTargetDistances from '../../../shared/components/dragDrop/currentTarget'

const getHeaders = () => screen.getAllByRole('columnheader')

jest.mock('../../../shared/api')
jest.mock('../../../shared/components/dragDrop/currentTarget', () => {
const actualModule = jest.requireActual(
'../../../shared/components/dragDrop/currentTarget'
)
return {
__esModule: true,
...actualModule
}
})

const { postMessage } = vsCodeApi
const mockPostMessage = jest.mocked(postMessage)
Expand Down Expand Up @@ -496,7 +503,12 @@ describe('ComparisonTable', () => {

const [, firstRow, secondRow] = screen.getAllByRole('rowgroup') // First rowgroup is the thead

dragAndDrop(secondRow, firstRow, DragEnterDirection.BOTTOM)
dragAndDrop(
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.

)

const [, newFirstRow, newSecondRow] = screen.getAllByRole('rowgroup')

Expand All @@ -509,7 +521,12 @@ describe('ComparisonTable', () => {

const [, firstRow, secondRow] = screen.getAllByRole('rowgroup') // First rowgroup is the thead

dragAndDrop(firstRow, secondRow, DragEnterDirection.BOTTOM)
dragAndDrop(
firstRow,
secondRow,
DragEnterDirection.BOTTOM,
EventCurrentTargetDistances
)

const [, newFirstRow, newSecondRow] = screen.getAllByRole('rowgroup')

Expand All @@ -535,7 +552,12 @@ describe('ComparisonTable', () => {

const [, firstRow, secondRow] = screen.getAllByRole('rowgroup') // First rowgroup is the thead

dragEnter(firstRow, secondRow.id, DragEnterDirection.BOTTOM)
dragEnter(
firstRow,
secondRow.id,
DragEnterDirection.BOTTOM,
EventCurrentTargetDistances
)

const dropTarget = screen.getAllByRole('rowgroup')[2]

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
/**
* @jest-environment jsdom
*/
import React from 'react'
import { render, cleanup, screen, fireEvent } from '@testing-library/react'
import '@testing-library/jest-dom/extend-expect'
Expand Down
Loading