Bugfix: ensure backend api routes are configured and respond to request #30
Conversation
…ensure instance can reference path
- Make FE request to /api/v1/health and append class to root
- requires setting expected API base url env var that differs from other params, but that is expected by the front-end API client
- accommodate successDataPath config that expects data to be nested at data.attributes in response
- I initially used a smaller payload, but this successDataPath was resulting in empty responses, which was confusing
- update e2e test to assert on presence of class on root element, as this is an actual end to end test
- we can update the health check once the DB is completely setup, but this was the minimal amount of work that will protect us in CI
| .DS_STORE | ||
|
|
||
| /build | ||
| /node_modules | ||
| /be/.env | ||
| /be/**/logs/* | ||
| be/node_modules/ | ||
| fe/node_modules/ | ||
|
|
||
| # e2e test output | ||
| # e2e test output and artifacts | ||
| cypress/screenshots/ |
There was a problem hiding this comment.
Noticed these files crop up in the diff when running cypress locally
| } | ||
|
|
||
| buildResponse(req, res) { | ||
| res.status(200).json({ data: { attributes: { healthy: true } } }) |
There was a problem hiding this comment.
I initially used a smaller payload here, but the front-end api client was confusingly responding with undefined due to the successDataPath config we had setup, which assumes the data is nested under data.attributes.
| // it's not a fresh instance, as that essentially wipes out the routes | ||
| const router = Router() | ||
|
|
||
| export class HealthRoute { |
There was a problem hiding this comment.
I added this as a distinct route, as it was the lowest complexity setup that I was sure would be immediately viable in CI, as there's no dependency on the database for this route (and the DB isn't set up in TravisCI, which we'll roll with until we get AWS + Jenkins set up).
be/src/routes/user.routes.js
Outdated
| // memoize router so that when the consumes retrieve it through getter, | ||
| // it's not a fresh instance, as that essentially wipes out the routes | ||
| const router = Router() | ||
|
|
||
| export class UserRoute { | ||
| static path() { | ||
| get path() { | ||
| return '/users' | ||
| } | ||
| get router() { | ||
| return Router() | ||
| return router |
There was a problem hiding this comment.
These changes are actually what fixed the bug for this route and the health route above, as we had switched to static path and get router in #26. getters are functions, so it was rerunning the Router() code anytime a consumer used the getter, which was wiping out the routes that got declared, so we had no routes actually being set up.
The switch from static to get was vital, too, in that static assumes consumers will call the method on the class itself, rather than an instance of the class, so this.path was returning undefined.
There was a problem hiding this comment.
thanks for fixing, I completely miss understood get and static declarations
| it('renders a main section that successfully interacts with the API', () => { | ||
| cy.get('.backend-healthy main').should('exist') |
There was a problem hiding this comment.
This updated assertion, in combination with implementation code below, will protect us against the regression of the API routes being unavailable.
There was a problem hiding this comment.
It does point out to me that we should probably set up some lower level integration tests for our API endpoints, as faster tests could have caught this.
We'll just need to set up the non-trivial boilerplate of setting these HTTP tests up, which I'm happy to do later this week or start of next.
| @@ -5,6 +5,7 @@ development: | |||
| PROXIED_API_TOKEN: '' | |||
|
|
|||
| # === FE === | |||
| REACT_APP_API_URL: http://localhost:8080 | |||
There was a problem hiding this comment.
The API client code assumes this variable is set, and it's vital in development, as the FE and BE are served via different ports. In other envs, it's not actually necessary, as the API client then just uses the path, as it doesn't need a fully qualified URL due to the api being served by the same server as the frontend.
It does feel weird to me that we have so many ENV VARS declaring similar values/configs, so I've created #31.
This pull request fixes a subtle and confusing bug that snuck in our recent linter PR. Namely, it ensures that our routers are memoized in their getters, as shifting them to getter functions was wiping out the declared routes.
To protect against API routes failing again, I've updated the code to add a high-level integration with the backend, and updated an E2E test assertion that assures we're protected from this behavior regressing in the future.