Skip to content

Fix unittest discovery when using VS Code Insiders by using Inversify's skipBaseClassChecks option. #15049

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.

Sign up for GitHub

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

Jump to bottom
Merged
kimadeline merged 3 commits into from
Jan 4, 2021
Merged

Fix unittest discovery when using VS Code Insiders by using Inversify's skipBaseClassChecks option. #15049

kimadeline merged 3 commits into from
Jan 4, 2021

Conversation

@kimadeline
Copy link

@kimadeline kimadeline commented Dec 23, 2020 •
edited
Loading

For #14962

What skipBaseClassCheck does: https://github.com/inversify/InversifyJS/blob/master/wiki/container_api.md#skipbaseclasschecks

You can use this to skip checking base classes for the @Injectable property, which is especially useful if any of your @Injectable classes extend classes that you don't control (third party classes). By default, this value is false.

UnitTestSocketServer (which is causing the issue) extends from EventEmitter, a Node class we don't control.

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Has a news entry file (remember to thank yourself!).
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated.
  • Test plan is updated as appropriate.
  • package-lock.json has been regenerated by running npm install (if dependencies have changed).
  • The wiki is updated with any design decisions/details.

@kimadeline kimadeline self-assigned this Dec 23, 2020
@kimadeline kimadeline marked this pull request as ready for review December 23, 2020 19:06
karrtikr
karrtikr approved these changes Dec 23, 2020
View reviewed changes
Copy link

@karrtikr karrtikr left a comment

Choose a reason for hiding this comment

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

Interesting, does this actually fix the unit test issue for you?

@kimadeline
Copy link
Author

kimadeline commented Dec 23, 2020

Yep, and I checked, unit tests are still discovered when using VS Code stable, it doesn't seem to have broken anything as far as I can see.

@codecov-io
Copy link

codecov-io commented Dec 23, 2020

Codecov Report

Merging #15049 (2fe60cd) into main (91ff97f) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##            main   #15049   +/-   ##
======================================
  Coverage     65%      65%           
======================================
  Files        558      558           
  Lines      26231    26231           
  Branches    3728     3728           
======================================
  Hits       17155    17155           
  Misses      8381     8381           
  Partials     695      695

@kimadeline kimadeline merged commit 4d1037b into microsoft:main Jan 4, 2021
@kimadeline kimadeline deleted the 14962-unittest-macos branch January 4, 2021 19:24
@karrtikr karrtikr mentioned this pull request Feb 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@brettcannon brettcannon brettcannon approved these changes

@karrtikr karrtikr karrtikr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@kimadeline kimadeline

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kimadeline @codecov-io @brettcannon @karrtikr