-
Notifications
You must be signed in to change notification settings - Fork 4
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
Api unit tests #15048
Open
benmartin-coforma
wants to merge
35
commits into
master
Choose a base branch
from
api-unit-tests
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Api unit tests #15048
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
Code Climate has analyzed commit de8fc4b and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (90% is the threshold). This pull request will bring the total coverage in the repository to 82.5% (0.0% change). View more on Code Climate. |
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.
Description
This PR creates unit tests for the SEDS app-api service. There are three important things to note:
My goal was to reach 100% coverage with 0 changes to non-test files; this turned out to be impossible, because bugs exist. For each bug, I had to choose whether to leave those lines untested (falling short of the 100% coverage goal), or to fix it (falling short of the 0 other changes goal). I ended up fixing the 3 bugs I judged most important which were fixable with minimal changes, and leaving the rest as-is. Those bugs are in authorization.js, obtainUserByEmail.js, and obtainUserByUsername.js. In addition to code that was literally untestable, I also left some completely unused code uncovered. For example: local-user.js, generateEnrollmentTotals.js, and response-lib.js. In the end, I was able to reach 90% coverage - although I did have to write tests for the currently-unused email notification code.
This PR does not incorporate the new tests into the deployment pipeline; at the moment, the only way to run them is to invoke them yourself locally. It also does not incorporate the Code Climate diff coverage checks, so nothing will automatically stop you from merging a PR that deletes API tests, adds new API code without tests, or otherwise reduces coverage. We can and should add those workflow integrations, and I intend to do so in a future PR.
The tests in this PR use the
vitest
framework. It is notably notjest
, but there are very few differences when you get down to the level of writing test code. All the usual test methods likedescribe
andexpect
work exactly the same way. Here's what is different:jest
global, you usevi
instead. As invi.fn()
andvi.mock()
.describe()
,expect()
, and so on) must be imported in each file; they are not globally available.jest.requireActual()
has a cousin rather than a twin. See generateQuarterForms.test.js for an example of Vitest'simportOriginal()
.yarn test --watch
; by default it runs all tests and then exits. With Vitest, this is inverted: watching is the default. In order to run all tests and then exit, you must invokeyarn test --run
.Jest Runner
(which inserts a "Run" button in the editor directly above each individual test) doesn't work with Vitest, obviously. I ended up using the--testNamePattern
CLI option when I wanted to run a single test. I have not yet tried to debug tests, nor have I looked for a similar VSCode plugin.Related ticket(s)
N/A
How to test
yarn test --run --coverage
Notes
Pre-review checklist
Pre-merge checklist
Review
[ ] Design: This work has been reviewed and approved by design, if necessary[ ] Product: This work has been reviewed and approved by product owner, if necessarySecurity
If either of the following are true, notify the team's ISSO (Information System Security Officer).
[ ] These changes are significant enough to require an update to the SIA.[ ] These changes are significant enough to require a penetration test.