-
Notifications
You must be signed in to change notification settings - Fork 734
test(performance): make tests more deterministic by relying more on system counts #5786
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
Changes from 34 commits
3f765c5
5a0357a
e07593f
3c1f909
f5d6c60
2897aee
caf2be8
658ed5a
366b038
1786ff4
3b987b7
56819a5
a13a6c7
da6c8ce
b3619a9
f9ed46d
dbefbf0
685924c
0aa5546
b0b63ad
4fe7c7b
f5947c1
fda96df
10130a6
55ee15e
10236d8
a6eda5b
b8ed1f4
efe25af
74dc701
3a11a85
1a2017b
5e2d976
157c628
6ecc708
fdc2010
870a644
71f7eef
4a253da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,55 +8,89 @@ import * as sinon from 'sinon' | |
| import { performanceTest } from '../../shared/performance/performance' | ||
| import { createTestWorkspaceFolder, toFile } from '../../test/testUtil' | ||
| import path from 'path' | ||
| import { randomUUID } from '../../shared' | ||
| import { fs, randomUUID } from '../../shared' | ||
| import { collectFiles } from '../../shared/utilities/workspaceUtils' | ||
| import { getFsCallsUpperBound } from './utilities' | ||
| import { FileSystem } from '../../shared/fs/fs' | ||
|
|
||
| performanceTest( | ||
| // collecting all files in the workspace and zipping them is pretty resource intensive | ||
| { | ||
| linux: { | ||
| userCpuUsage: 85, | ||
| heapTotal: 2, | ||
| duration: 0.8, | ||
| function performanceTestWrapper(totalFiles: number) { | ||
| return performanceTest( | ||
| { | ||
| darwin: { | ||
| userCpuUsage: 100, | ||
|
||
| systemCpuUsage: 35, | ||
| heapTotal: 2, | ||
| }, | ||
| linux: { | ||
| userCpuUsage: 100, | ||
| systemCpuUsage: 35, | ||
| heapTotal: 2, | ||
| }, | ||
| win32: { | ||
| userCpuUsage: 100, | ||
| systemCpuUsage: 35, | ||
| heapTotal: 2, | ||
| }, | ||
| }, | ||
| }, | ||
| 'calculate cpu and memory usage', | ||
| function () { | ||
| const totalFiles = 100 | ||
| return { | ||
| setup: async () => { | ||
| const workspace = await createTestWorkspaceFolder() | ||
| 'calculate cpu and memory usage', | ||
| function () { | ||
| return { | ||
| setup: async () => { | ||
| const workspace = await createTestWorkspaceFolder() | ||
|
|
||
| sinon.stub(vscode.workspace, 'workspaceFolders').value([workspace]) | ||
| sinon.stub(vscode.workspace, 'workspaceFolders').value([workspace]) | ||
| const fsSpy = sinon.spy(fs) | ||
| const findFilesSpy = sinon.spy(vscode.workspace, 'findFiles') | ||
| const fileContent = randomUUID() | ||
| for (let x = 0; x < totalFiles; x++) { | ||
| await toFile(fileContent, path.join(workspace.uri.fsPath, `file.${x}`)) | ||
| } | ||
|
|
||
| const fileContent = randomUUID() | ||
| for (let x = 0; x < totalFiles; x++) { | ||
| await toFile(fileContent, path.join(workspace.uri.fsPath, `file.${x}`)) | ||
| } | ||
| return { | ||
| workspace, | ||
| fsSpy, | ||
| findFilesSpy, | ||
| } | ||
| }, | ||
| execute: async ({ workspace }: { workspace: vscode.WorkspaceFolder }) => { | ||
| return { | ||
| result: await collectFiles([workspace.uri.fsPath], [workspace], true), | ||
| } | ||
| }, | ||
| verify: ( | ||
| setup: { | ||
| workspace: vscode.WorkspaceFolder | ||
| fsSpy: sinon.SinonSpiedInstance<FileSystem> | ||
| findFilesSpy: sinon.SinonSpy | ||
| }, | ||
| { result }: { result: Awaited<ReturnType<typeof collectFiles>> } | ||
| ) => { | ||
| assert.deepStrictEqual(result.length, totalFiles) | ||
| const sortedFiles = [...result].sort((a, b) => { | ||
| const numA = parseInt(a.relativeFilePath.split('.')[1]) | ||
| const numB = parseInt(b.relativeFilePath.split('.')[1]) | ||
| return numA - numB | ||
| }) | ||
| for (let x = 0; x < totalFiles; x++) { | ||
| assert.deepStrictEqual(sortedFiles[x].relativeFilePath, `file.${x}`) | ||
| } | ||
|
|
||
| return { | ||
| workspace, | ||
| } | ||
| }, | ||
| execute: async ({ workspace }: { workspace: vscode.WorkspaceFolder }) => { | ||
| return { | ||
| result: await collectFiles([workspace.uri.fsPath], [workspace], true), | ||
| } | ||
| }, | ||
| verify: ( | ||
| _: { workspace: vscode.WorkspaceFolder }, | ||
| { result }: { result: Awaited<ReturnType<typeof collectFiles>> } | ||
| ) => { | ||
| assert.deepStrictEqual(result.length, totalFiles) | ||
| const sortedFiles = [...result].sort((a, b) => { | ||
| const numA = parseInt(a.relativeFilePath.split('.')[1]) | ||
| const numB = parseInt(b.relativeFilePath.split('.')[1]) | ||
| return numA - numB | ||
| }) | ||
| for (let x = 0; x < totalFiles; x++) { | ||
| assert.deepStrictEqual(sortedFiles[x].relativeFilePath, `file.${x}`) | ||
| } | ||
| }, | ||
| assert.ok( | ||
| getFsCallsUpperBound(setup.fsSpy) <= totalFiles * 5, | ||
| 'total system calls below 5 per file' | ||
| ) | ||
| assert.ok(setup.findFilesSpy.callCount <= 2, 'findFiles not called more than twice') | ||
| }, | ||
| } | ||
| } | ||
| } | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| describe('collectFiles', function () { | ||
| afterEach(function () { | ||
| sinon.restore() | ||
| }) | ||
| performanceTestWrapper(10) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this approach of simplifying it this way! |
||
| performanceTestWrapper(100) | ||
| performanceTestWrapper(250) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: AdmZip will be replaced by zip.js #4769
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I see. Do you recommend I remove the AdmZip call counting in the meantime or leave it until that other PR is done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went ahead and deleted the AdmZip work. Once a new zip is implemented, this can be augmented to spy on it as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#4769 is merged now