test: FORMS-1444 add public and utils routes tests (#1477)

* test: FORMS-1444 add public and utils routes

* for consistency check userAccess.currentUser

* docs: add some thoughts around the route testing

* docs: added brief notes on middleware testing

* refactor: updated the proxy routes tests for consistency

app/tests/unit/README.md

@@ -53,3 +53,36 @@ describe('my tests', () => {
 ```
 
 Similar to `.only` is the `.skip` modifier to skip a test or group of tests.
+
+## Testing Strategy
+
+The testing strategy for the backend unit tests can be broken down into the different layers of the backend. For all tests we should:
+
+- ensure that the tests are consistent
+- ensure that we have 100% test coverage
+- ensure that we have complete test coverage: we should be testing additional corner cases even once we reach 100% test coverage
+- test the interface, not the implementation
+- test the unit under test, not its dependencies
+
+### Middleware Testing
+
+The tests for the middleware files should:
+
+- mock all services calls used by the middleware, including both exception and minimal valid results
+- test all response codes produced by the middleware
+
+### Route Testing
+
+The tests for the `route.js` files should:
+
+- mock all middleware used by the file
+- each route test should check that every middleware is called the proper number of times
+- each route test should mock the controller function that it calls
+- mock controller functions with `res.sendStatus(200)`, as doing something like `next()` will call multiple controller functions when route paths are overloaded
+- check that the mocked controller function is called - this will catch when a new route path accidentally overloads an old one
+- for consistency and ease of comparison, alphabetize the expect clauses ("alphabetize when possible")
+
+Note:
+
+- Some middleware is called when the `routes.js` is loaded, not when the route is called. For example, the parameters to `userAccess.hasFormPermissions` cannot be tested by calling the route using it. Even if we reload the `routes.js` for each route test, it would be hard to tell which call to `hasFormPermissions` was the call for the route under test
+- Maybe we should refactor and create a set of standard middleware mocks that live outside the `routes.spec.js` files

app/tests/unit/forms/form/externalApi/routes.spec.js

@@ -83,6 +83,7 @@ describe(`${basePath}/:formId/externalAPIs`, () => {
     expect(controller.listExternalAPIs).toBeCalledTimes(1);
     expect(hasFormPermissionsMock).toBeCalledTimes(1);
     expect(rateLimiter.apiKeyRateLimiter).toBeCalledTimes(0);
+    expect(userAccess.currentUser).toBeCalledTimes(1);
     expect(validateParameter.validateExternalAPIId).toBeCalledTimes(0);
     expect(validateParameter.validateFormId).toBeCalledTimes(1);
   });
@@ -98,6 +99,7 @@ describe(`${basePath}/:formId/externalAPIs`, () => {
     expect(controller.createExternalAPI).toBeCalledTimes(1);
     expect(hasFormPermissionsMock).toBeCalledTimes(1);
     expect(rateLimiter.apiKeyRateLimiter).toBeCalledTimes(0);
+    expect(userAccess.currentUser).toBeCalledTimes(1);
     expect(validateParameter.validateExternalAPIId).toBeCalledTimes(0);
     expect(validateParameter.validateFormId).toBeCalledTimes(1);
   });
@@ -125,6 +127,7 @@ describe(`${basePath}/:formId/externalAPIs/:externalAPIId`, () => {
     expect(controller.deleteExternalAPI).toBeCalledTimes(1);
     expect(hasFormPermissionsMock).toBeCalledTimes(1);
     expect(rateLimiter.apiKeyRateLimiter).toBeCalledTimes(0);
+    expect(userAccess.currentUser).toBeCalledTimes(1);
     expect(validateParameter.validateExternalAPIId).toBeCalledTimes(1);
     expect(validateParameter.validateFormId).toBeCalledTimes(1);
   });
@@ -152,6 +155,7 @@ describe(`${basePath}/:formId/externalAPIs/:externalAPIId`, () => {
     expect(controller.updateExternalAPI).toBeCalledTimes(1);
     expect(hasFormPermissionsMock).toBeCalledTimes(1);
     expect(rateLimiter.apiKeyRateLimiter).toBeCalledTimes(0);
+    expect(userAccess.currentUser).toBeCalledTimes(1);
     expect(validateParameter.validateExternalAPIId).toBeCalledTimes(1);
     expect(validateParameter.validateFormId).toBeCalledTimes(1);
   });
@@ -219,6 +223,7 @@ describe(`${basePath}/:formId/externalAPIs/statusCodes`, () => {
     expect(controller.listExternalAPIStatusCodes).toBeCalledTimes(1);
     expect(hasFormPermissionsMock).toBeCalledTimes(1);
     expect(rateLimiter.apiKeyRateLimiter).toBeCalledTimes(0);
+    expect(userAccess.currentUser).toBeCalledTimes(1);
     expect(validateParameter.validateExternalAPIId).toBeCalledTimes(0);
     expect(validateParameter.validateFormId).toBeCalledTimes(1);
   });