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

Write "Working with Extensions / Bundling Extension" topic #2254

Closed
octref opened this issue Dec 2, 2018 · 42 comments
Closed

Write "Working with Extensions / Bundling Extension" topic #2254

octref opened this issue Dec 2, 2018 · 42 comments
Assignees
Labels
extensibility extension author content under /api
Milestone

Comments

@octref
Copy link
Contributor

octref commented Dec 2, 2018

Related: microsoft/vscode-extension-samples#122

@octref octref added the extensibility extension author content under /api label Dec 2, 2018
@octref octref added this to the Extensibility Backlog milestone Dec 2, 2018
@DanTup
Copy link
Contributor

DanTup commented Jan 3, 2019

Please also include a section for tests (both running from VS Code via launch.json and if much different, running from the command line for CIs), as I was unable to get tests working when I updated my npm scripts to use webpack (see microsoft/vscode-extension-samples#110 (comment)).

@KamasamaK
Copy link
Contributor

Related #2403

@jrieken
Copy link
Member

jrieken commented Feb 14, 2019

@octref Do you want webpack as separate guide? My initial planning was to put this into api/working-with-extensions/publishing-extension.md so that it is not more prominent/easier to find. My thinking was the the base-publishing guide should prepare you for the webpack-guide

@octref
Copy link
Contributor Author

octref commented Feb 14, 2019

I think separate topic (api/working-with-extensions/bundling-extension.md) is better:

  • It's on TOC, so even easier to find
  • The publishing guide could link to the bundling guide in "Next Steps", you can also put it right after publishing guide in TOC

@jrieken
Copy link
Member

jrieken commented Feb 27, 2019

fyi - I have pushed a first cut via 1023304

@DanTup
Copy link
Contributor

DanTup commented Feb 27, 2019

@jrieken If you're looking for feedback, a section on tests would be appreciated. I couldn't figure out how to make tests work after changing the extension to be packed (specifically, I wasn't sure what the launch.json/packages.json should look like to run them with F5 from VS Code, or from the terminal).

@jrieken
Copy link
Member

jrieken commented Feb 27, 2019

Yeah, that looks very hard-coded: https://github.com/Microsoft/vscode-extension-vscode/blob/master/bin/test#L44. @bpasero is there a way to make this configurable?

@DanTup
Copy link
Contributor

DanTup commented Feb 27, 2019

@jrieken I didn't even get that far (see microsoft/vscode-extension-samples#110 (comment)). As part of switching to webpack the call to tsc was replaced with a call to webpack, so the tests weren't even being compiled anywhere. I wasn't sure how it was supposed to work (I've never used webpack before, so I wasn't really sure where to start with it).

@jrieken
Copy link
Member

jrieken commented Feb 27, 2019

so the tests weren't even being compiled anywhere

That's a good sign because it means your source doesn't depend on your tests ;-) For test keep compiling with tsc or compile another webpack entry that's your tests, tho I think the former makes most sense. I will update the sample and guide

@gregvanl
Copy link

gregvanl commented Feb 27, 2019

@jrieken I did an edit pass, added metadata, and placed the topic in the TOC with 4886d4e
In a later commit, I moved Bundling Extension above Continuous Integration.

@gregvanl
Copy link

Will be published when 1.32 released

@DanTup
Copy link
Contributor

DanTup commented Feb 28, 2019

@jrieken did you make updates about testing? I can't see them in the changesets mentioned above, but if they exist somewhere, I'd like to have a go at trying to migrate my project :-)

@octref
Copy link
Contributor Author

octref commented Mar 1, 2019

Let's keep this open until there's a section on testing. In the old tsc compilation you get a js file emitted each ts file so you can write unit tests easily. We should provide some documentation on how to do unit testing with webpack.

@octref octref reopened this Mar 1, 2019
jrieken added a commit that referenced this issue Mar 4, 2019
@jrieken
Copy link
Member

jrieken commented Mar 4, 2019

I have added a section about running tests - basically suggesting to keep things as they are because it doesn't make sense to bundle unit tests.

@octref This will be published under https://code.visualstudio.com/api/working-with-extensions/bundling-extension, correct? Asking because I have created a fwd link for vsce (microsoft/vscode-vsce#336)

@jrieken
Copy link
Member

jrieken commented Mar 4, 2019

@gregvanl Please do review on the new section. Thanks

@DanTup
Copy link
Contributor

DanTup commented Mar 4, 2019

@jrieken

basically suggesting to keep things as they are because it doesn't make sense to bundle unit tests

To run tests a simple compile is suffienct, in the sample a test-compile-script exists. It invokes the TypeScript-compiler which compiles into the out folder.

Would this run the tests against webpacked extension code? If not, I don't think that's a good solution.

@jrieken
Copy link
Member

jrieken commented Mar 4, 2019

If not, I don't think that's a good solution.

It's a unit test for a portion of your code. It's not an integration test and it's not testing webpack. There should be no difference, webpack'ed or not. It's like saying your tests should be run against TypeScript and not against the JavaScript that tsc produces.

@DanTup
Copy link
Contributor

DanTup commented Mar 4, 2019

It's a unit test for a portion of your code. It's not an integration test and it's not testing webpack

Many people do (and should) have integration tests for their extensions. But also, testing against the output of webpack doesn't mean you're just testing webpack - you're also making sure you haven't written any code that Webpack (incorrectly) strips out because it thinks is unused (understand that's a possibility, for ex. with dynamic imports?).

Though FWIW, I don't think "testing webpack" is a bad thing. Projects often have bugs, and sometimes they slip through their own tests. I'd rather deliver something I know works than something broken where I can just say that it's an upstream bug. (In the same way that my extension's integration tests have caught regressions in VS Code before now).

I understand if there's no good way to do this (or you think it not worthwhile), but it could prevent people moving to packed extensions (it'll certainly stop me). It'd be nice if it was supported.

@jrieken
Copy link
Member

jrieken commented Mar 4, 2019

I understand if there's no good way to do this (or you think it not worthwhile), but it could prevent people moving to packed extensions (it'll certainly stop me). It'd be nice if it was supported.

Obviously there a way of achieving this but it's outside the scope of a simple tutorial. You seem to have a strong opinion on this, so this is the webpack-doc: https://webpack.js.org/concepts. It will explain you how this works.

@DanTup
Copy link
Contributor

DanTup commented Mar 4, 2019

Obviously there a way of achieving this but it's outside the scope of a simple tutorial.

Sure, but I don't think it should be out of scope of the guidance given by VS Code for extension authors. I did spend many hours trying to make this work and couldn't figure it out, so I posted here back in November ago asking for help/guidance. The issue I'd commented on was closed without covering this and I ultimately ended up on this one. This one also seems like it's not going to cover it. I don't think the Webpack docs help, because the problem is knowing how to run the tests (which are started by VS Code's script) with the packed version of the extension loaded (this is also done by VS Code, the tests don't load the extension). Maybe it's simple, I don't know - but I couldn't figure it out and there doesn't seem to be anywhere else I could ask this and get a reasonable answer (it's VS Code-specific).

Honestly, I'm don't really know if there's a benefit to packing it. I assumed there was because you were working on it, but you know what they say about assumptions. I'll leave it unpacked for now.

@jrieken
Copy link
Member

jrieken commented Mar 4, 2019

In microsoft/vscode-extension-samples#110 (comment) you have asked what to with tests. We have provided an answer, including a sample. You disagree with our suggestion which is fine but then you are also a little on your own.

I did spend many hours trying to make this work and couldn't figure it out

Sorry, I don't believe that. I think you are smart enough to come up with a concrete, real question instead of "tried it, didn't work, what's next" question. If you have an issue with how the runner works (which is a single, non-minified JS file) we are happy to help. If you have a concrete question with webpack I might be able to help, but ultimately I am also a user that reads their docs.

@DanTup
Copy link
Contributor

DanTup commented Mar 4, 2019

Sorry, I don't believe that.

I often don't believe how many hours I spend on things, but sadly that doesn't make them not true 😞

I think you are smart enough to come up with a concrete, real question instead of "tried it, didn't work, what's next" question.

I didn't post "tried it, didn't work, what's next". I gave details about what I tried and why I got stuck:

I'm not sure how launch.json and scripts in package.json should be set up when using webpack. Copying the webpack example I end up removing the call to tsc in favour of calling webpack, which results in my extension being built (into dist/extension.js) but no tests being compiled.

I understand a little better now, but at the time I assumed I needed to build a packed version that included the test entry point. I've never used Webpack before, and this is my only TS project. I'm also a lot less familiar with VS Code than you are. I was probably going in the wrong direction with what I was trying to do, but that's why I came asking for help - that's the only thing I can do if I hit a wall.

Anyway, I have a better idea about it now, maybe I can get it to work by running tsc and webpack to run the unpacked tests but loading the packed extension. I may come back to it at some point and have a go. I still think it'd be a good idea if the samples were a good basis for people getting started though (and didn't steer them away from having integration tests).

@octref
Copy link
Contributor Author

octref commented Mar 4, 2019

This will be published under https://code.visualstudio.com/api/working-with-extensions/bundling-extension, correct?

The link is correct.

@DanTup Can't you have 2 entry points, one for the extension build and one for the test build, and then use the --extensionTestsPath to point to the test build output?

@DanTup
Copy link
Contributor

DanTup commented Mar 4, 2019

Can't you have 2 entry points, one for the extension build and one for the test build, and then use the --extensionTestsPath to point to the test build output?

Do you mean two Webpack entry points? I don't know.. The docs for webpack do mention multiple entry points (https://webpack.js.org/concepts/entry-points/) but it's not clear how that'd work - if I ended up with two files, the test one may end up with a copy of all the extension code it calls inside it, and if it ends up with one, it isn't clear how to pass that to VS Code (since it needs two things - extension + tests).

I think what I wrote in the last paragraph above will work - run two compiles - a webpack one for the extension, and a tsc one for tests, then pass those two outputs in the VS Code config/commands. I'll give it a go when I have some time to debug it if it doesn't work. It's not all that high priority :)

@octref
Copy link
Contributor Author

octref commented Mar 4, 2019

it isn't clear how to pass that to VS Code (since it needs two things - extension + tests).

--extensionDevelopmentPath and --extensionTestsPath in here https://code.visualstudio.com/api/working-with-extensions/testing-extension#launch-tests-configuration.

Can you give a concrete example of what functionality you want to test, which the current webpack config doesn't support?

@DanTup
Copy link
Contributor

DanTup commented Mar 4, 2019

@octref I mean, if everything is compiled into a single file (tests + extension) how you'd pass both those things.

Can you give a concrete example of what functionality you want to test

There isn't specific functionality I want to test, I wanted to enable webpack and have all my existing tests (whatever they do) - and any future tests - work the same, but using the packed version of the extension (I incorrectly assumed that's exactly what everybody that packed the extension would want to do).

Dart-Code/Dart-Code@6ca65b9 are the changes I made to pack the extension (which worked), but I couldn't figure out how to set up launch.json to run the tests. I think I have a better idea now - I think I need a new npm script that both tsc's the tests, and webpack's the extension, and then use that as the preLaunchScript for the test configs (and then do something similar in my CI test script). I'll try it out when I have some spare time anyway.

@DanTup
Copy link
Contributor

DanTup commented Mar 5, 2019

Ok, so I had a go at what I wrote above by having a new script that did both webpack and tests:

"webpack-with-tests": "webpack --mode development && tsc -p ./",

Then used that as the preLaunchTask for my test configs. I managed to get some of my tests passing, however it seems to falls down in a several places because any classes that have state are now duplicated (in the tests, and in the packed extension code) and also there are two definitions of classes referenced by tests. This means something as simple as item instanceof SuiteTreeItem in a test fails, because the instance that came from the extension has its own definition of SuiteTreeItem that isn't the same as the one in the test (well, it is the same, but it's a copy).

So, maybe it's not realistic to run any integration tests this way. I think the tests would need packing to reference the same shared code to work. Unfortunately, I don't know how to do that and this has already consumed a bunch of time for which the benefit is still questionable, so for now I'm going to just stick without packing the extension. Thanks for the pointers anyway!

@octref
Copy link
Contributor Author

octref commented Mar 5, 2019

Here's my take:

  • If your tests depend directly on your source code, those are unit tests. Test them with your test runner and don't webpack them.
  • If your tests do not depend on your source code, those are integration tests. You can webpack them into two entries for 2 output bundle files and point to them with --extensionDevelopmentPath and --extensionTestsPath.
  • Ultimately if nothing works for you, you can use webpack as a pre publish step and do not use it for testing and development.

It would be great if you can fork https://github.com/Microsoft/vscode-extension-samples/tree/master/webpack-sample and demonstrate to us the problem. Like "Clone this repo, run npm i && npm test, this test should pass but failed".

@DanTup
Copy link
Contributor

DanTup commented Mar 5, 2019

If your tests depend directly on your source code, those are unit tests
If your tests do not depend on your source code, those are integration tests

I've got integration tests that do depend on my source code. It's a convenient way to verify state in places where it's hard to otherwise verify the results. Yes, I'm sure I can refactor them to work differently, but that would probably involve adding more APIs just to support my tests. I'm already exposing some things through the extensions exports, though even that broke here (because I used a Symbol() to hide it from the public API, and now my extension/tests can't share a Symbol()) and I hate to expose them publicly.

Ultimately if nothing works for you, you can use webpack as a pre publish step and do not use it for testing and development.

I'd prefer to just not pack the extension than do that. Running all the tests against one set of code, then running it through something that includes removing unused code before shipping to users seems like an unnecessary risk IMO. My tests exist to verify what I'm shipping to users, so I'd like to keep them the same.

It would be great if you can fork

Tbh, I've already spent many more hours on this than I wanted to and it very often feels like there's resistance to support integration testing (generally, not just this issue). Since I was already a little unsure of the advantages of packing the extension and now it's turning out to be non-trivial, I don't think it's worthwhile. There are better things we could all be spending time on :-)

Thanks for the effort anyway, and apologies for derailing this thread!

@johnpapa
Copy link
Contributor

johnpapa commented Mar 13, 2019

In case it helps anyone else, here is what I managed to get working. I'm not 100% sure this is the right way to go ... any feedback is appreciated.

My goals were to make all of these still work:

  • packaging with npm run package
  • publishing with npm run publish
  • local and CI testing with npm run test
  • F5 debugging with the launch.json
  • F5 debugging the tests with the launch.json

The approach has me compiling both with webpack and tsc for the tests and debugging.

Here is my project https://github.com/johnpapa/vscode-peacock

Changed my main file in package.json

  "main": "./dist/extension",

My npm scripts in package.json

  "scripts": {
    "package": "npx vsce package",
    "publish": "npx vsce publish",

    "vscode:prepublish": "webpack --mode production",
    "compile": "webpack --mode none",
    "watch": "webpack --mode none --watch",

    "postinstall": "node node_modules/vscode/bin/install",
    "just-test": "node node_modules/vscode/bin/test",
    "test-compile": "tsc -p ./ && npm run compile",
    "test": "npm run test-compile && node node_modules/vscode/bin/test"
  },

My launch.json configurations for debugging the runtime and tests:

  "configurations": [
    {
      "name": "Run Extension",
      "type": "extensionHost",
      "request": "launch",
      "runtimeExecutable": "${execPath}",
      "args": ["--extensionDevelopmentPath=${workspaceFolder}"],
      "outFiles": ["${workspaceFolder}/dist/**/*.js"],
      "preLaunchTask": "npm: test-compile"
    },
    {
      "name": "Extension Tests",
      "type": "extensionHost",
      "request": "launch",
      "runtimeExecutable": "${execPath}",
      "args": [
        "${workspaceFolder}/testworkspace",
        "--disable-extensions",
        "--extensionDevelopmentPath=${workspaceFolder}",
        "--extensionTestsPath=${workspaceFolder}/out/test"
      ],
      "outFiles": ["${workspaceFolder}/out/test/**/*.js"],
      "preLaunchTask": "npm: test-compile"
    }
  ]

@egamma
Copy link
Member

egamma commented Mar 13, 2019

@jrieken FYI

@jrieken
Copy link
Member

jrieken commented Mar 13, 2019

@johnpapa re microsoft/vscode-extension-samples#163 can you be more specific? The webpack-sample has a test-compile-script and launch.json - Extension Test. For me it runs tests and prints the results in the Debug Output panel.

@johnpapa
Copy link
Contributor

Yes - the sample in github and what are in the doc are not the same. It took me a bit to figure that out.

But really - I'm wondering if what I did above is proper. I'm not a webpack guru :)

The odd part for me is ... (from above) ...

The approach has me compiling both with webpack and tsc for the tests and debugging.

Is that intended? debugging and tests fail if I dont. Or do I have it setup wrong?

@jrieken
Copy link
Member

jrieken commented Mar 13, 2019

Is that intended? debugging and tests fail if I dont. Or do I have it setup wrong?

One should be enough: for debugging, use the webpack-compile (the extension starts from dist/extension). for testing, use the tsc-compile (tests are discovered in out/test/**)

Yes - the sample in github and what are in the doc are not the same. It took me a bit to figure that out.

Good catch - the test paragraph only mentions the added script but doesn't show an updated sections. @gregvanl what the easiest/fastest way to update the doc?

@octref
Copy link
Contributor Author

octref commented Mar 13, 2019

@gregvanl
Copy link

@jrieken Is this all that's needed 3c3d689?

@jrieken
Copy link
Member

jrieken commented Mar 14, 2019

Yeah - lgtm

@johnpapa
Copy link
Contributor

One should be enough: for debugging, use the webpack-compile (the extension starts from dist/extension). for testing, use the tsc-compile (tests are discovered in out/test/**)

I am testing both of these out. First, before trying either I delete the out and dist folders, just to make sure I am not using old compilations.

@jrieken Thanks for the tips. I tried using just the tsc script for testing and I get this error

extensionHostProcess.js:319
Here is the error stack:  Error: Cannot find module '/Users/papa/_git/vscode-peacock/dist/extension'

when i try just the webpack compile script for debugging, that works

so it appears I just need to fix something with the test setup. Any ideas?

@johnpapa
Copy link
Contributor

@frenya
Copy link

frenya commented Apr 30, 2020

It seems to me there's one fundamental flaw when it comes to unit testing while using the suggested webpack configuration:

  • The extension code that is loaded and activated by the extension host is the one pointed to by the main attribute of package.json, i.e. dist/extension.js
  • Any module you reference in your unit tests will be loaded from the out folder

This is not a good solution. For instance, you will run into problems in case your tests rely on any of those modules being initialised by the activate function (e.g. when you want to reference the extension context).

We need a way enforce loading the extension from out/extension.js for unit tests. Editing package.json every time is not a good option.

@DanTup
Copy link
Contributor

DanTup commented May 4, 2020

@frenya that's similar to the issue I described in #2254 (comment). You end up with two copies of everything, so things like instanceof won't work, and global state will have two copies, etc.

To work around this, I did

  1. Expose many of the extensions implementations to tests by returning an API from the activate method
    https://github.com/Dart-Code/Dart-Code/blob/49affded32a8c741d8cace1b94e32e3311b2854d/src/extension/extension.ts#L613-L643
    Unfortunately using Symbol() doesn't work here for the same reason, so the API becomes visible to anyone that wants to use it (I found a few other extensions calling my internals here despite trying to make it clear in the name it was intended to be a private API)
  2. Add lints to ensure that no test files import any of the extension code directly
    https://github.com/Dart-Code/Dart-Code/blob/49affded32a8c741d8cace1b94e32e3311b2854d/lints/disallowImportingNonSharedCodeRule.js

It's not ideal, but it's been working for me.

@frenya
Copy link

frenya commented May 4, 2020

Thanks @DanTup for the workaround. I actually came up with a different workaround in the meantime - posted it here #3632.

It uses a global json tool to modify package.json just for the execution of unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extensibility extension author content under /api
Projects
None yet
Development

No branches or pull requests

8 participants