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

Refactoring. Introduction of vscode abstraction layer & unit tests #16

Closed

Conversation

se02035
Copy link
Contributor

@se02035 se02035 commented Apr 20, 2022

Related to issue #17

Refactoring. Improved testability and maintainability by:

  • introducing a vscode abstraction layer
  • moved logic into different service classes (incl. dependency injection)
  • introducing unit tests incl code coverage (in addition to existing integration tests)

This PR likely needs to be updated once the other PRs have been merged. But this PR contains the basis for these future update and should outline the approach and recommended changes.

@se02035 se02035 marked this pull request as ready for review April 20, 2022 14:53
@The-Compiler
Copy link
Owner

I agree with the sentiment in microsoft/vscode#94746 that it should be easier to unit test VS Code extensions - ideally, with some kind of testing interface or mocks from the same source the official VS Code APIs come from. Until that's reality, however, I don't think it's in the scope of this extension to solve this problem.

I also prefer integration tests running against a "real" VS Code when possible: First and foremost, this is a VS Code extension heavily using the framework we get from VS Code - so I think it makes a lot of sense to test it in its "natural habitat". How it integrates with VS Code is a big part of what it does, and the parts of the code which can be run outside of VS Code are really small and simple. Almost all of the complexity (especially with the recent PRs) stems from better/different integration with VS Code functionality.

I think the drawbacks and additional complexity introduced by such an abstraction layer outweigh the benefits (which are certainly there, and it wasn't an easy decision!). Also consider e.g. a newcomer to Typescript and VS Code extensions who wants to contribute to this extension (or... myself 😅): Having an additional abstraction layer which needs to be implemented/extended for any VS Code functionality in use can be a quite heavy additional obstacle to contributions.

All in all, given the rather small complexity and scope of this extension, I'd prefer taking a more "lightweight"/fine-grained approach, where we mock and/or use dependency injection where it's needed for testability (such as in cb9817d).

After thinking about this for a while I'm afraid I don't think I like this approach. I'm therefore going to close this, but thanks for the contribution anyways - I appreciate the focus on testing as well as the documentation outlining the approach you took!

@se02035
Copy link
Contributor Author

se02035 commented Jul 8, 2022

Thanks @The-Compiler for your comments. I agree with you that this adds an extra level of complexity. That said, there is a difference between unit tests and integration tests (using the underlaying VSCode infrastructure). Unit tests won't replace integration tests: both types are there for very specific reasons. Unit tests should be considered as an extra benefit for the developers.

Unit tests are typically the first set of tests devs run: they should be fast and external systems should be abstracted (decoupled) as much as possible. They should help to increase developer velocity. if a unit test fails it doesn't make sense to run other activities that potentially are more expensive (performance and/or cost) activities.

The focus is really to test a single unit instead of a bigger flow (integration). Unit tests make it easy to identify "potential" problematic code areas (e.g. if it is difficult to write an unit test for a specific component - thats typically a good indicator that the code base should be refactored - and overall codebase maintainability will increase over time). Let me give you another example: the VSCode integration tests make it really hard to mock certain areas (this is per-se not a problem of your code - but the way how the VSCode libraries are developed in combination with the supported testing tools/lib makes mocking harder). So, writing fully automated tests is not always as easy as it could be. So, decoupling your code from the VSCode integration code will definitively help here. Plus, unit tests are a great tool to document the actual code base (a highly underestimated value of unit tests). You can achieve similar things with integration tests, but they are not as targeted (to a single unit, function/method, etc) as unit tests.

But I get your point that this change introduces some complexity. Based on my experience I can tell: the importance of unit tests is directly correlated to (not a complete list):

  1. to the size of the code base
  2. number of code contributors

But again, thanks for reviewing the PR and your feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants