test: use assert.rejects and drop chai #194
Conversation
google-cla
bot
added
the
cla: yes
| }); | ||
| it('Forwards GitHub error if createTree fails', async () => { | ||
| // setup | ||
| const createTreeErrorMsg = 'Error committing'; |
There was a problem hiding this comment.
Ok this requires an explanation :) With try/catch test pattern here, the intent was to ensure the code throws an error, and then check the error message. That only works if you put an assert.fail after the code that you expect to throw. Turns out ... these tests never worked as intended. The handleTree method here and below never actually threw, so when I converted them to assert.rejects, the tests started failing. Since they never worked as intended, I just went ahead and removed them. We can come back around to improve coverage.
Codecov Report
@@ Coverage Diff @@
## master #194 +/- ##
==========================================
+ Coverage 87.88% 88.04% +0.16%
==========================================
Files 24 24
Lines 2303 2301 -2
Branches 152 168 +16
==========================================
+ Hits 2024 2026 +2
+ Misses 279 275 -4
Continue to review full report at Codecov.
|
| }); | ||
| it('fails when there is no env variable', async () => { | ||
| try { | ||
| sandbox.stub(process.env, 'ACCESS_TOKEN').value(undefined); |
There was a problem hiding this comment.
Another fun fact! This code was throwing when it tried to create the stub, because ACCESS_TOKEN didn't exist as a property. This test was bunk.
| // tests | ||
| const changes = parseTextFiles(userFilesMap); | ||
| expect(changes).to.deep.equal(parsedMap); |
There was a problem hiding this comment.
I now fully distrust the deep equal implementation in Chai. The returned object was actually a set with class instances, not naked objects.
Apologies for the size of the review. This PR does a few things in pursuit of test cleanup:
chai, and uses the built inassertmoduleassert.throwsandassert.rejectsinstead of try/catch gymnasticstslintignore tags