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

feat(foam-vscode): add jest for testing #204

Merged
merged 3 commits into from
Aug 4, 2020

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Aug 2, 2020

This adds all the necessary pieces for Jest to work properly in foam-vscode. It also enables testing to run as part of the GitHub workflow.

Copy link
Collaborator

@jevakallio jevakallio left a comment

Choose a reason for hiding this comment

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

@jsjoeio thank you for starting this work! I've just pushed a failing commit onto this branch to demonstrate an issue we need to address.

The problems is that the vscode dependency is injected by the VS Code extension host environment (not sure how they provide it; possibly a custom node.require implementation, or a runtime-injected module). This means that any module that imports vscode can not be imported into a jest test without mocking the vscode module first.

Here's an issue on vscode-test with some workarounds. I'm not sure which is the best approach, haven't had time to investigate in depth. Happy to entertain any option.
microsoft/vscode-test#37

Beyond simple unit tests, here's resources on how we could run VSCode e2e tests with Jest:
https://medium.com/@soloydenko/end-to-end-testing-vs-code-extensions-via-jest-828e5edfeb75

The former needs to be addressed before it makes sense to land this. The latter can be left out of scope for further enhancement.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Aug 2, 2020

@jevakallio Those are good points! The intention behind this was to have the bare minimal of Jest set up so that we could at least write unit tests.

This means that any module that imports vscode can not be imported into a jest test without mocking the vscode module first.

Right! Thanks for bringing that up. My thinking was to add support for those types of integration and e2e tests later. But if you want that to all land as one unit in this PR, happy to do some investigation (thanks for the resources btw!).

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Aug 2, 2020

After doing a quick investigation, it seems like mocking the vscode module is the most common approach. There is an ongoing discussion about making this easier for folks.

For now, I'm going to work on adding a mock then I'll re-request your review.

Comment on lines +1 to +16
/*
Note: this is needed in order to test certain parts
of functionality of `foam-vscode`

Following the advice from this article:
https://www.richardkotze.com/coding/unit-test-mock-vs-code-extension-api-jest

combined with advice from this GitHub issue comment:
https://github.com/microsoft/vscode-test/issues/37#issuecomment-584744386
*/

const vscode = {
// Add values and methods as needed for tests
};

module.exports = vscode;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was enough to get the test you pushed to pass. Thoughts?

@jsjoeio jsjoeio requested a review from jevakallio August 2, 2020 19:31
Copy link
Collaborator

@riccardoferretti riccardoferretti left a comment

Choose a reason for hiding this comment

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

I think this is a really good step forward!
Even if we can't access vscode, we can still limit its code to the extension.ts file as suggested in one of the links you included (or at least limit its creep into the BL code).

Looks good to me!

Copy link
Collaborator

@jevakallio jevakallio left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 Thanks @jsjoeio!

@jevakallio jevakallio merged commit 7f3ba78 into master Aug 4, 2020
@jevakallio jevakallio deleted the jsjoeio-add-testing-foam-vscode branch August 4, 2020 12:27
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.

3 participants