-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[plug-in] enable running of VS Code extension tests #4231
Conversation
@@ -117,6 +118,9 @@ export class HostedPluginProcess implements ServerPluginRunner { | |||
env.PATH = process.env.PATH; | |||
// add HOME to env since some plug-ins need to read files from user's home dir | |||
env.HOME = process.env.HOME; | |||
if (argv.extensionTestsPath) { |
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.
it's not overkill to use bring yargs dependency there ?
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.
yargs
should be always available
but I better to replace it with CliContribution
that we parse args only once
|
||
// Execute the runner if it follows our spec | ||
if (testRunner && typeof testRunner.run === 'function') { | ||
return new Promise<void>((c, e) => { |
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.
minor: could we use (resolve, reject) instead of (c, e) ?
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.
ok
@@ -120,10 +122,13 @@ export class PluginManagerExtImpl implements PluginManagerExt, PluginManager { | |||
if (pluginMain !== undefined) { | |||
this.startPlugin(plugin, configStorage, pluginMain); | |||
} else { | |||
return Promise.reject(new Error('Unable to load the given plugin')); | |||
console.error(`Unable to load a plugin from "${plugin.pluginPath}"`); |
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.
should the below if (his.host.loadTests) be part of else if block instead ?
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.
The issue here that VS Code extension contributes only to package.json and stub main
property with an inexistent file or just skip it. It causes an exception and prevents loading other plugins and tests.
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.
ok that's not really nice to specify main
to a file that does not exist :-/
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
62c51ff
to
edb34af
Compare
@benoitf i've addressed your comments, could you have a look again please? |
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.
thx @akosyakov
fix #4226
Changes needed to run VS Code API unit tests: https://github.com/theia-
ide/theia/issues/4224#issuecomment-459752557