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

Run test options: Polish APIs #74560

Closed
aeschli opened this issue May 29, 2019 · 5 comments · Fixed by microsoft/vscode-test#21
Closed

Run test options: Polish APIs #74560

aeschli opened this issue May 29, 2019 · 5 comments · Fixed by microsoft/vscode-test#21
Assignees
Labels
extensions Issues concerning extensions on-testplan polish Cleanup and polish issue
Milestone

Comments

@aeschli
Copy link
Contributor

aeschli commented May 29, 2019

Testing #74436

Create a TS extension as described in #74436 and look at runTest.ts. It invokes runTests from vscode-tests .
https://github.com/microsoft/vscode-test/blob/master/lib/runTest.ts

With vscode-tests becoming more prominent, its APIs should have same quality as our vscode extension APIs. Here are some areas we need to improve before releasing to the public:

  • vscodeExecutablePath: I assume this is the path to the VSCode Electron exe, not the cli command. An usage example would be helpful.

TestOptions

  • extensionPath: If I understand right, this maps to 'extensionDevelopmentPath'. Since last milestone we now also support defining multiple extension dev paths. So maybe this should be <string | string[]>
  • testRunnerPath: See runTests.d.ts spec not consistent with code #74555
  • testWorkspace: Is this only for .code-workspace files, or any kind of input (folder, folders, files)? Is it a path or a URI? How do I specify an empty window? Given the many possibilities for inputs I suggest to remove that property in favour of letting the user define these in launchArgs
  • additionalLaunchArgs. Is the example listed in the spec useful? What happens if you define that list but also define extensionPath and testWorkspace ?

ExplicitTestOptions
are much cleaner. Do we really want to provide both TestOptions & ExplicitTestOptions?
It avoids some of the problems listed above, I'd suggest to remove TestOptions and just have ExplicitTestOptions.
For convenience we can always add factory methods that create a ExplicitTestOptions. That's IMO the better approach than the TestOptions interface.

runTests
Needs specs. What is the number that is returned?

@jrieken jrieken assigned octref and bpasero and unassigned jrieken May 29, 2019
@bpasero bpasero removed their assignment May 29, 2019
@octref octref added the under-discussion Issue is under discussion for relevance, priority, approach label May 29, 2019
@octref
Copy link
Contributor

octref commented May 29, 2019

vscodeExecutablePath: I assume this is the path to the VSCode Electron, not the cli commands. An Usage example would be helpful.

That's in the readme: https://github.com/microsoft/vscode-test#usage

    /**
     * Manually download VS Code 1.30.0 release for testing.
     */
    const vscodeExecutablePath = await downloadAndUnzipVSCode('1.30.0')
    await runTests({
      vscodeExecutablePath,
      extensionPath,
      testRunnerPath,
      testWorkspace
    })

extensionPath...<string | string[]>

I think that can be a feature request, but not blocking release. 1) You can use explicit launchArgs 2) Originally vscode doesn't have this functionality either.

testWorkspace

Files/Folders/path-to-workspace-files. The first argument passed to code executable. I can improve the doc a bit.

additionalLaunchArgs. Is the example listed in the spec useful? What happens if you define that list but also define extensionPath and testWorkspace ?

Then it's like calling code --extensionDevelopmentPath <path> --extensionDevelopmentPath <path>. I think it's useful, see https://github.com/microsoft/vscode-docs/blob/vnext/api/working-with-extensions/testing-extension.md#disabling-other-extensions-while-debugging.

ExplicitTestOptions

I think we do. Not everyone wants to write --extensionDevelopmentPath, etc as plain strings. I think TestOptions is good for most people and ExplicitTestOptions is really for advanced people.

@Tyriar @rebornix What do you think? Should we drop the simple API?

Needs specs. What is the number that is returned?

I can add docs.

@octref octref added this to the May 2019 milestone May 29, 2019
@aeschli
Copy link
Contributor Author

aeschli commented May 29, 2019

Files/Folders/path-to-workspace-files. The first argument passed to code executable. I can improve the doc a bit.

Workspace is not a good name for that. Maybe pathsToOpen. Can be one, multiple, none. Can be paths, URI (--file-uri...). Give the many variants (and who knows what's in the future), I think it's better the user just uses launchArgs for that.

@rebornix
Copy link
Member

ExplicitTestOptions and TestOptions are not clear to me, ExplicitTestOptions is a subset of TestOptions and the doc doesn't tell what's the major difference between the two.

For most people, IMO they should only write mandatory options and then testing can work as expected. Optional options are advanced users, they need to understand TestOptions and tweak as wanted. Right now there are actually three tiers,

  • Users who previously didn't need to customize anything, now they need to specify extensionPath
  • Users who wants to add additional launch args, change workspace, etc
  • Users who wants to take control of all options and use ExplicitTestOptions

It would be great if we can simply them. If it's not practical, at least we document better.

@Tyriar
Copy link
Member

Tyriar commented May 29, 2019

The initial idea of splitting them was to make things as simple as possible for the 90%+ of users. We generate this in the yeoman generator don't we, maybe that's simple enough already if we replace TestOptions with ExplicitTestOptions?

@octref octref modified the milestones: May 2019, June 2019 May 30, 2019
@aeschli
Copy link
Contributor Author

aeschli commented Jun 4, 2019

Created branch https://github.com/microsoft/vscode-generator-code/tree/vscode-test-and-types-at-code with all the commits.
Set master back to the previous version to avoid that the changes get pushed inadvertently.

octref added a commit to microsoft/vscode-test that referenced this issue Jun 21, 2019
@octref octref added extensions Issues concerning extensions on-testplan polish Cleanup and polish issue and removed under-discussion Issue is under discussion for relevance, priority, approach labels Jun 24, 2019
@vscodebot vscodebot bot locked and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
extensions Issues concerning extensions on-testplan polish Cleanup and polish issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants