All new code requires tests to ensure against regressions.
However, if your PR contains zero code changes, feel free to select the checkmark below to indicate so.
[X] Have you ran tests against this code?
[ ] This PR contains zero code changes.
Description of the Change
This PR mainly focuses on decoupling HTTP handling from the logic of the backend. This makes not only the backend easier to reason through, easier to contribute to (by the way of not having to know ExpressJS APIs or logic for everything), but also can greatly make testing even easier.
This is done, by again relying on ServerStatusObjects from the functions called by main.js instead of handling HTTP returns within functions.
This also means that to test these functions we can call then as normal functions and rely on the data returned. Rather than create mock ExpressJS req and res objects and inspect their data after the function to determine behavior, and also means we no longer have to call the endpoints via supertest to determine behavior.
Additionally, this PR put a focus on passing modules, rather than requiring them.
While this change is much less noticeable, the idea again is to be able to test much more easily. As mocks are messy, difficult to reason with, and require a bit of setup and teardown per usage, we can instead pass functions as needed and have much much greater control over the data supplied and consumed within these tests, much more easily than the same with mocks.
On passing as formal arguments rather than requiring, the basic idea should be that any module that deals with a codebase boundary (as defined by ARCHITECTURE.md, such as database calls, superagent calls, the VCS service, and auth service) should be passed instead of required. So that way instead of having to mock the module being required we can pass whatever we want, and craft any functions and data returns we prefer. This potentially means being able to directly return whatever data we want from database calls, rather than have to inject more and more test data into our test database. It also means much easier error checking as we can return purposefully failed database calls, which can never be reproduced honestly in our current testing strategy.
There was some thought given to instead of passing each module as a formal argument, we instead passed an object containing the modules required per function, e.g.:
async function getPackages(params, mods) {
await mods.db.getPackages();
}
But it was thought this could create an effect of having to many layers to each parameter, since already knowing what keys are available within the params object requires either:
Accurate parameter documentation within the JSDoc comment above the function
Reading the source code of main.js to determine exactly what is being passed
So to avoid doing this twice, and with something like modules, the tradeoff of having more modules didn't seem like to big of an issue.
Some documentation must be updated soon to describe the best practices here, since that can be the best way to ensure uniformity with how this is done. Such as passing the formal parameters in this exact order: (params, database, auth) rather than another.
Requirements
Filling out the template is required.
All new code requires tests to ensure against regressions.
[X] Have you ran tests against this code?
[ ] This PR contains zero code changes.
Description of the Change
This PR mainly focuses on decoupling HTTP handling from the logic of the backend. This makes not only the backend easier to reason through, easier to contribute to (by the way of not having to know ExpressJS APIs or logic for everything), but also can greatly make testing even easier.
This is done, by again relying on ServerStatusObjects from the functions called by
main.jsinstead of handling HTTP returns within functions.This also means that to test these functions we can call then as normal functions and rely on the data returned. Rather than create mock ExpressJS
reqandresobjects and inspect their data after the function to determine behavior, and also means we no longer have to call the endpoints viasupertestto determine behavior.Additionally, this PR put a focus on passing modules, rather than requiring them. While this change is much less noticeable, the idea again is to be able to test much more easily. As mocks are messy, difficult to reason with, and require a bit of setup and teardown per usage, we can instead pass functions as needed and have much much greater control over the data supplied and consumed within these tests, much more easily than the same with mocks.
On passing as formal arguments rather than requiring, the basic idea should be that any module that deals with a codebase boundary (as defined by
ARCHITECTURE.md, such as database calls, superagent calls, the VCS service, and auth service) should be passed instead of required. So that way instead of having to mock the module being required we can pass whatever we want, and craft any functions and data returns we prefer. This potentially means being able to directly return whatever data we want from database calls, rather than have to inject more and more test data into our test database. It also means much easier error checking as we can return purposefully failed database calls, which can never be reproduced honestly in our current testing strategy.There was some thought given to instead of passing each module as a formal argument, we instead passed an object containing the modules required per function, e.g.:
But it was thought this could create an effect of having to many layers to each parameter, since already knowing what keys are available within the
paramsobject requires either:main.jsto determine exactly what is being passedSo to avoid doing this twice, and with something like modules, the tradeoff of having more modules didn't seem like to big of an issue.
Some documentation must be updated soon to describe the best practices here, since that can be the best way to ensure uniformity with how this is done. Such as passing the formal parameters in this exact order:
(params, database, auth)rather than another.