-
Notifications
You must be signed in to change notification settings - Fork 87
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
[AC-1743] pt. 2.5 ⮕ Fix the unit tests for good #398
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mimartin12
previously approved these changes
Jan 10, 2024
eliykat
requested changes
Jan 11, 2024
2693722
to
daa7d7c
Compare
7416d80
to
9f8be98
Compare
eliykat
approved these changes
Jan 11, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
joseph-flinn
approved these changes
Jan 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[AC-1743] pt. 2.5 ⮕ Fix the unit tests for good
Type of change
🏁 Objective
As part of an initiative to audit dependencies across this project we have been blocked by issues running tests. We need tests to consistently pass to audit used vs unused test dependencies.
🚩 The Problem
Oscar helped get tests passing at the start of this effort, but I broke them when merging the first part of the work to unpackage jslib. This went unnoticed until yesterday, and is blocking the next phase of doing a thorough dependency audit.
🏃The Solution
It was difficult to tell what exactly was going wrong with these tests. Any curious jest experts that want to take a better stab at understanding the break can run
npm test
frommain
to observe the current behavior. The core issue, to me, seems to have been the change totsconfig.paths
structures leading to jest not properly mapping imports. This theory has holes in it, since that configuration is automated by ats-jest
helper, but the first batch of errors onmain
does point in the same direction.Regardless, I dove through a few sets of errors and ultimately just decided to rebuild the jest config from scratch. I used the documentation for
ts-jest
,angular-preset-jest
, andjest
as guidelines, as well as the currentclients
config and the existing config in directory-connectormain
.The end result produces a fully passing test suite. I deleted a few suites that were failing at the end of the work, but only after confirming those suites did not have relevant code for Directory Connector and were instead leftovers from the
clients
monorepo split andjslib
orphaning. These includedWebCryptoFunctionService
tests, which are used to compare encryption keys, and this is not relevant to Directory Connector. It also includesutils
tests for domain matching, which again is only relevant to the core Bitwarden product line.In addition I went ahead and added a Github workflow for running tests in CI. This was a quick copy/paste/prune job from
clients
, but it seems to cover our needs for this project.⏭️ What's next?
Next up in this dependency upgrade effort I'll be:
Testing requirements
There are no specific testing requirements for this. QA will be doing a regression run of Directory Connector at the end of this effort.
Before you submit
npm run lint
) (required)