Skip to content
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

chore: setup scaffolding for backend tests #940

Merged
merged 15 commits into from
May 4, 2021
Merged

Conversation

miazima
Copy link
Contributor

@miazima miazima commented Jan 7, 2021

Setup base scaffolding for backend tests.

The main testing framework used is jest. I considered using mocha and chai as alternative, but concluded on jest so that we use a common testing framework across frontend and backend.

Testing related frameworks/libraries used:

  • Jest
  • SuperTest (facilitates testing HTTP endpoints)
  • redis-mock

Addresses #880

Mocking database queries
sequelize-mock does not actually carry out db queries. Instead we mock query results by queuing the expected results e.g. UserMock.$queueResult()

Database queries
In the previous implementation, I had used sequelize-mock to mock sequelize query return values. After discussion with the team, we decided that database queries were a essential part and it would be better to test them with real queries instead.

Mocking user session
A valid user session is introduced by intercepting the request and modifying req.session object. This is used for all routes that require authentication.

  • This may not be the ideal solution is it doesn't allow us to write tests with the presence and absence of a valid user session as needed.
  • The auth routes does not have a user session mocked which makes testing auth/userinfo difficult as it is not possible to test with and without an user session

Improvements:

  • include tests folder in tsconfig.json and exclude from build files
  • move all tests to tests folder
  • add tests for authentication and campaign routes endpoints

Tests

Run all tests npm test

Deploy Notes

New dev dependencies:

  • supertest
  • redis-mock
  • @types/redis-mock
  • @types/supertest

@miazima miazima force-pushed the backend-tests branch 2 times, most recently from 371e162 to f3110fa Compare January 13, 2021 04:26
@miazima miazima marked this pull request as ready for review January 13, 2021 04:52
@miazima miazima force-pushed the backend-tests branch 2 times, most recently from ddaccb4 to a6be190 Compare February 17, 2021 02:57
@lamkeewei lamkeewei self-requested a review March 8, 2021 02:29
Copy link
Contributor

@lamkeewei lamkeewei left a comment

Choose a reason for hiding this comment

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

When running the tests, the following warning is raised:

A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --runInBand --detectOpenHandles to find leaks.

We might want to look into this.

Another thing to note is that jest runs tests in different files in parallel unless --runInBand is set. This might be an issue if we have tests in each file interfering with each other. E.g. Test in auth deletes a user with email test@test.com while another test in campaign tries to create a campaign belonging to test@test.com.

One way this can be solved is to ensure that each test suite runs in a separate namespace-ed db (e.g. postmangovsg_test_auth) by passing an argument to seqeulizeLoader. This db can be torn down at the end of each test suite. We want to avoid setting --runInBand due to the performance hit to tests since all test suite are now run serially.

Found this link that might be useful. They namespace-ed the test database based on the Jest worker ID.

backend/tests/setup.ts Outdated Show resolved Hide resolved
backend/tests/routes/server.ts Outdated Show resolved Hide resolved
.travis.yml Outdated
Comment on lines 21 to 22
- psql -c 'create database postmangovsg_test_1;' -U postgres
- psql -c 'create database postmangovsg_test_2;' -U postgres
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am manually creating test databases here instead of generating them dynamically, I also restricted the number of jest workers to be the same number of test dbs created

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to do this initialisation and the subsequent teardown in the globalSetup and the globalTeardown step? Just thinking along of lines of making it easier when running in dev so that he/she wouldn't have to remember to create/drop the two test db.

backend/package.json Show resolved Hide resolved
Copy link
Contributor

@lamkeewei lamkeewei left a comment

Choose a reason for hiding this comment

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

lgtm! Left a minor remark about the creation of the test databases. Noted that this is just the scaffold, so I didn't check if all the required flows were addressed.

backend/src/core/routes/tests/auth.routes.test.ts Outdated Show resolved Hide resolved
@miazima
Copy link
Contributor Author

miazima commented Apr 28, 2021

I've refactored the initial setup to include a globalSetup file, however environmental variables are not loaded during globalSetup so I defined them within the file.

@miazima
Copy link
Contributor Author

miazima commented May 4, 2021

@lamkeewei made some changes to the db setup, could you have a final look before we merge this

@lamkeewei
Copy link
Contributor

@lamkeewei made some changes to the db setup, could you have a final look before we merge this

lgtm! Tested the creation of test database in global setup function and works as expected.

@lamkeewei lamkeewei merged commit aaaaa73 into develop May 4, 2021
@lamkeewei lamkeewei deleted the backend-tests branch May 4, 2021 03:05
lamkeewei added a commit that referenced this pull request May 5, 2021
* develop:
  refactor: use shared function to initialize models (#1172)
  chore: setup scaffolding for backend tests (#940)
  1.23.0
  fix(frontend): fix frontend test flakiness (#1162)
  fix: update successful delivery status only if error does not exist (#1150)
  chore: upgrade dependencies (#1153)
  feat: add unit tests for error states in critical workflows (#1118)
  feat: support whitelisting domains through `agencies` table (#1141)
  feat: add tests for happy paths in critical workflows (#1110)
  fix: prevent campaign names from causing dashboard rows to overflow (#1147)
  fix(email): Fix SendGrid fallback integration (#1026)
lamkeewei added a commit that referenced this pull request May 5, 2021
* develop:
  feat: refactor msg template components; add telegram character limit (#1148)
  refactor: use shared function to initialize models (#1172)
  chore: setup scaffolding for backend tests (#940)
  1.23.0
  fix(frontend): fix frontend test flakiness (#1162)
  fix: update successful delivery status only if error does not exist (#1150)
  chore: upgrade dependencies (#1153)
  feat: add unit tests for error states in critical workflows (#1118)
  feat: support whitelisting domains through `agencies` table (#1141)
  feat: add tests for happy paths in critical workflows (#1110)
  fix: prevent campaign names from causing dashboard rows to overflow (#1147)
lamkeewei added a commit that referenced this pull request May 10, 2021
* develop:
  fix: fix error when updating Telegram ID for an existing phone number (#1178)
  chore: upgrade React; use new JSX transform; sort imports (#1129)
  fix(backend): docker build and tsc output directory structure (#1177)
  feat: refactor msg template components; add telegram character limit (#1148)
  refactor: use shared function to initialize models (#1172)
  chore: setup scaffolding for backend tests (#940)
  1.23.0
  fix(frontend): fix frontend test flakiness (#1162)
  fix: update successful delivery status only if error does not exist (#1150)
  chore: upgrade dependencies (#1153)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants