Skip to content

(test): fix glob mode match and remove duplicate loader.js in renderer.html #183507

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.

Sign up for GitHub

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

Closed
yiliang114 wants to merge 9 commits into from
Closed

(test): fix glob mode match and remove duplicate loader.js in renderer.html #183507

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5fabf0e
(test): fix glob mode match and remove duplicate loader.js in render…
yiliang114 May 26, 2023
239fa54
(test): fix glob mode match and remove duplicate loader.js in render…
yiliang114 May 26, 2023
19df6dd
(test): fix glob mode match and remove duplicate loader.js in render…
yiliang114 May 26, 2023
daf1bc2
Merge branch 'main' into unwilling-cattle
yiliang114 Jun 13, 2023
6021de5
Merge branch 'main' into unwilling-cattle
yiliang114 Jun 16, 2023
d862297
Merge branch 'main' into unwilling-cattle
yiliang114 Jul 18, 2023
35f06a0
Merge branch 'main' into unwilling-cattle
yiliang114 Aug 15, 2023
6872d18
Merge branch 'main' into unwilling-cattle
yiliang114 Aug 16, 2023
34f628c
Merge branch 'main' into unwilling-cattle
yiliang114 Oct 8, 2023
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
3 changes: 2 additions & 1 deletion src/vs/workbench/browser/parts/sidebar/sidebarPart.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,8 @@ export class SidebarPart extends CompositePart<PaneComposite> implements IPaneCo
themeService,
Registry.as<PaneCompositeRegistry>(ViewletExtensions.Viewlets),
SidebarPart.activeViewletSettingsKey,
viewDescriptorService.getDefaultViewContainer(ViewContainerLocation.Sidebar)!.id,
// During unit testing, it may be empty and needs.
viewDescriptorService.getDefaultViewContainer(ViewContainerLocation.Sidebar)?.id || '',
Copy link
Copy Markdown
Member

@connor4312 connor4312 May 26, 2023

Choose a reason for hiding this comment

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

Why is this needed? Are any tests currently failing because of this?

Copy link
Copy Markdown
Contributor Author

@yiliang114 yiliang114 May 26, 2023

Choose a reason for hiding this comment

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

Why is this needed? Are any tests currently failing because of this?

Now the existing unit tests in VS Code are not executed into the SidebarPart constructor, so this writing (use ! rather than ? ) does not cause unit test execution to fail. The reason I encountered unit test failure is that I did not use TestSidebarPart , but directly use SidebarPart.

vscode/src/vs/workbench/test/browser/workbenchTestServices.ts

Line 663 in 89db3e4

this.parts.set(ViewContainerLocation.Sidebar, new TestSideBarPart());

When I need to initialize SidebarPart, it will report an error because I haven't registered any other views at the time of unit testing.

Another reason is that other modules are fetched using optional chain processing, because there is no guarantee that there will be a default view container.

image

'sideBar',
'viewlet',
SIDE_BAR_TITLE_FOREGROUND,
Expand Down
4 changes: 2 additions & 2 deletions test/unit/browser/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ const { applyReporter } = require('../reporter');
// opts
const defaultReporterName = process.platform === 'win32' ? 'list' : 'spec';
const optimist = require('optimist')
// .describe('grep', 'only run tests matching <pattern>').alias('grep', 'g').alias('grep', 'f').string('grep')
.describe('build', 'run with build output (out-build)').boolean('build')
.describe('run', 'only run tests matching <relative_file_path>').string('run')
.describe('glob', 'only run tests matching <pattern>').string('glob')
Copy link
Copy Markdown
Member

@connor4312 connor4312 May 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This should be runGlob to match our other unit test types, and be described as "only run test files matching..."

Copy link
Copy Markdown
Contributor Author

@yiliang114 yiliang114 May 26, 2023

Choose a reason for hiding this comment

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

This should be runGlob to match our other unit test types, and be described as "only run test files matching..."

emmmm, but in

vscode/test/unit/README.md

Line 24 in 89db3e4

- to run only a subset of tests use the `--run` or `--glob` options
and

vscode/test/unit/browser/index.js

Line 84 in 89db3e4

// glob patterns (--glob)
, It seems that the supported parameter is --glob. Has it been abandoned?

Copy link
Copy Markdown
Member

@connor4312 connor4312 May 26, 2023

Choose a reason for hiding this comment

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

It looks like in the electron test we allow both glob and runGlob, so I guess that's fine. I would add a similar .alias('glob', 'runGlob') just for consistency's sake

.describe('grep', 'only run tests matching <pattern>').alias('grep', 'g').alias('grep', 'f').string('grep')
.describe('debug', 'do not run browsers headless').alias('debug', ['debug-browser']).boolean('debug')
.describe('sequential', 'only run suites for a single browser at a time').boolean('sequential')
Expand Down Expand Up @@ -83,7 +83,7 @@ const testModules = (async function () {
} else {
// glob patterns (--glob)
const defaultGlob = '**/*.test.js';
const pattern = argv.run || defaultGlob;
const pattern = (argv.glob || defaultGlob).replace(new RegExp(`^(${out}|src)/`), '').replace(/\.ts$/, '.js');
isDefaultModules = pattern === defaultGlob;

promise = new Promise((resolve, reject) => {
Expand Down
48 changes: 29 additions & 19 deletions test/unit/browser/renderer.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,28 +40,38 @@
});
</script>

<!-- Depending on --build or not, load loader from known locations -->
<script src="../../../out/vs/loader.js"></script>
<script src="../../../out-build/vs/loader.js"></script>

<script>
const isBuild = urlParams.get('build');

// configure loader
const baseUrl = window.location.href;
require.config({
catchError: true,
baseUrl: new URL('../../../src', baseUrl).href,
paths: {
vs: new URL(`../../../${!!isBuild ? 'out-build' : 'out'}/vs`, baseUrl).href,
assert: new URL('../assert.js', baseUrl).href,
sinon: new URL('../../../node_modules/sinon/pkg/sinon.js', baseUrl).href,
'sinon-test': new URL('../../../node_modules/sinon-test/dist/sinon-test.js', baseUrl).href,
xterm: new URL('../../../node_modules/xterm/lib/xterm.js', baseUrl).href,
'@vscode/iconv-lite-umd': new URL('../../../node_modules/@vscode/iconv-lite-umd/lib/iconv-lite-umd.js', baseUrl).href,
jschardet: new URL('../../../node_modules/jschardet/dist/jschardet.min.js', baseUrl).href
}
});
function loaderScript(href) {
return new Promise((resolve, reject) => {
const script = document.createElement('script');
script.src = href;
script.onload = () => resolve();
script.onerror = () => reject();
document.head.appendChild(script);
});
}

(async () => {
await loaderScript(`../../../${!!isBuild ? 'out-build' : 'out'}/vs/loader.js`);

// configure loader
const baseUrl = window.location.href;
require.config({
catchError: true,
baseUrl: new URL('../../../src', baseUrl).href,
paths: {
vs: new URL(`../../../${!!isBuild ? 'out-build' : 'out'}/vs`, baseUrl).href,
assert: new URL('../assert.js', baseUrl).href,
sinon: new URL('../../../node_modules/sinon/pkg/sinon.js', baseUrl).href,
'sinon-test': new URL('../../../node_modules/sinon-test/dist/sinon-test.js', baseUrl).href,
xterm: new URL('../../../node_modules/xterm/lib/xterm.js', baseUrl).href,
'@vscode/iconv-lite-umd': new URL('../../../node_modules/@vscode/iconv-lite-umd/lib/iconv-lite-umd.js', baseUrl).href,
jschardet: new URL('../../../node_modules/jschardet/dist/jschardet.min.js', baseUrl).href
}
});
})();
</script>

<script>
Expand Down