Conversation
ezhangy
changed the title
ezhangy
changed the title
ezhangy
left a comment
ezhangy
left a comment
There was a problem hiding this comment.
Annotated changes with comments!
| let stack: Stack; | ||
| let template: Template; | ||
|
|
||
| beforeEach(() => { |
There was a problem hiding this comment.
moved the logic in the beforeEach to the createStackAndTemplate function, which allows us to mock functions called in the Stack within a test.
also refactored the function to return the stack and template rather than assigning them to variables to avoid any unexpected behavior due to mutability
| const app = new App(); | ||
| const currentWorkingDir = path.basename(path.resolve(process.cwd())); | ||
| const rootDir = path.resolve(__dirname, '../../'); | ||
| const rootDir = path.basename(path.resolve(__dirname, '../../')); |
There was a problem hiding this comment.
without adding path.basename, the rootDir was the absolute path while currentWorkingDir was only the directory name, causing the conditional below to always fail
| }); | ||
|
|
||
| it('creates an SNS subscription for Slack endpoint', () => { | ||
| const valueForStringParameterMock = jest.spyOn( |
There was a problem hiding this comment.
in the previous PR, I refactored the code to get the subscription email from SSM instead of hard-coding it and forgot to update this test
| MOCK_SSM_CLIENT.on(GetParameterCommand).resolves({ | ||
| Parameter: { | ||
| Value: null | ||
| Value: null as unknown as string | undefined |
There was a problem hiding this comment.
Value is of type string | undefined, but we're purposely testing passing the wrong type here. The casting is necessary to avoid a type error.
| const auth = new google.auth.JWT( | ||
| clientEmail, | ||
| null, | ||
| undefined, |
There was a problem hiding this comment.
This argument (it's the keyFile parameter) was causing a type error because it was previously set to null but has type string | undefined.
I changed it to undefined and double-checked that it didn't affect the API by deploying to dev and sending some test requests.
|
[boulder|dust] Unless there's a reason not to, I think the GitHub CI should not only be running tests on PRs and commits to https://github.com/newjersey/feedback-api/blob/dev/.github/workflows/ci.yml For example, on this PR for making tests pass, the CI doesn't run tests. I trust you ran them locally and they work, but having that nice ✅ makes it even more instantly clear (and in some cases might catch something local tests don't). |
yes, such a good point! i think i had mentally filed away our entire CI/CD pipeline in a future ticket about implementing automatic CDK deployments, but you're totally right it's easy to just add the tests to PRs on the dev branch now 🙂 will do! |
mluedke2
left a comment
mluedke2
left a comment
There was a problem hiding this comment.
thanks for adding the tests onto dev too!
sannidhishukla
left a comment
sannidhishukla
left a comment
There was a problem hiding this comment.
looks good to me! added a few comments about adding tests but like i said, feel free to address these later if it doesn't make sense to put time into setting these up right now!
3 participants
This PR fixes the tests written in #75 as well as longer-standing type errors that were causing our test suite to fail.
Changes in files are annotated with comments.