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

Adding unit tests - READY :D #60

Merged

Conversation

katelynienaber
Copy link
Contributor

@katelynienaber katelynienaber commented Nov 9, 2020

This PR includes the following:

  • Test runner, so you can start the unit tests from the extension host

Some minor Lint fixes, where I saw them:

  • Ordering object properties alphabetically
  • Trailing commas

Unit tests for:

  • Profiles refresh function
  • Profiles getEndevorCliProfileManager function
  • List (view) action for elements (browseElement)
  • retrieveElement
  • retrieveElementWithDependencies
  • createNewConnection

I did not test promptCredentials because it is currently not called anywhere in the extension...

katelynienaber and others added 9 commits November 6, 2020 12:35
Added unit test config to launch.json

Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
…ent with dependencies)

Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
@katelynienaber katelynienaber marked this pull request as ready for review November 11, 2020 12:20
@katelynienaber katelynienaber changed the title Adding unit tests Adding unit tests - READY Nov 11, 2020
@Alexandru-Dumitru Alexandru-Dumitru self-assigned this Nov 18, 2020
Copy link

@Alexandru-Dumitru Alexandru-Dumitru left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. Some things I noticed:

  • we can keep tests close to the functionality they test, so we don't need src/__tests__/__unit__, but we can put them in /functionality/__tests__/. Tests for functionality under src can obviously go in src/__tests__/ (not really mandatory, just a thought)
  • I tried to run the tests both from npm run test:no-vscode and the debugger, and didn't work for me. I got the following error for all new test files:
    Cannot find module 'vscode' from 'src/__tests__/__unit__/service/profiles-test.ts'  

      26 |  */
      27 | 
    > 28 | import * as vscode from "vscode";
  • also, once we add meaningful tests, we can remove the fake ones at src/__tests__/sample-test.ts and src/__tests__/sampletests folder.
  • lastly, fixing linting stuff adds a little bit of noise to the review process :( . We are also going to replace linting tool within other PR (so rules will change). No big issue here
  • Ran the extension manually from the branch and it worked fine for me (manual testing) 👍

.vscode/launch.json Outdated Show resolved Hide resolved
@zdmullen
Copy link
Contributor

Hey, I got several errors when I ran the tests. Here are some screenshots:
image
image
image

Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
@katelynienaber
Copy link
Contributor Author

katelynienaber commented Nov 19, 2020

@Alexandru-Dumitru @zdmullen Okay so unit tests should be fixed now.

Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
@katelynienaber katelynienaber changed the title Adding unit tests - READY Adding unit tests - READY :D Nov 19, 2020
Copy link

@Alexandru-Dumitru Alexandru-Dumitru left a comment

Choose a reason for hiding this comment

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

Thanks Katelyn! Works well for me. 👍

I am going to approve it but I do have one remark going forward. The reason why we use Mocha and vscode-test is to test the integration with VS Code. Having to extensively mock vscode stuff to make Jest tests work is something that we should strive to avoid. This means that through an ongoing refactoring process we should localize as much as possible the usage of vscode module so that it doesn't create this kind of impediment in tests.

@Alexandru-Dumitru Alexandru-Dumitru merged commit 318a8fd into eclipse-che4z:development Nov 19, 2020
@Alexandru-Dumitru Alexandru-Dumitru deleted the add-unit-tests branch November 19, 2020 16:11
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.

None yet

3 participants