-
Notifications
You must be signed in to change notification settings - Fork 0
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
Bugfix: ensure backend api routes are configured and respond to request #30
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,13 @@ | ||
.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/ | ||
nohup.out | ||
/tmp/**/* |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,11 @@ | ||
|
||
|
||
import { App } from './app.js'; | ||
import { HealthRoute } from './src/routes/health.routes.js'; | ||
import { UserRoute } from './src/routes/user.routes.js'; | ||
|
||
// ValidateEnv(); | ||
|
||
const app = new App([new UserRoute()]); | ||
const app = new App([new HealthRoute(), new UserRoute()]); | ||
|
||
app.listen(); | ||
app.listen(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
// TODO: add swagger docs. get health and E2E test working first. | ||
|
||
import { Router } from 'express' | ||
|
||
// 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 HealthRoute { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
get path() { | ||
return '/health' | ||
} | ||
|
||
get router() { | ||
return router | ||
} | ||
|
||
constructor() { | ||
this.initializeRoutes() | ||
} | ||
|
||
buildResponse(req, res) { | ||
res.status(200).json({ data: { attributes: { healthy: true } } }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I initially used a smaller payload here, but the front-end api client was confusingly responding with |
||
} | ||
|
||
initializeRoutes() { | ||
this.router.get(`${this.path}`, this.buildResponse) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,12 +60,16 @@ | |
import { Router } from 'express' | ||
import { UsersController } from '../controllers/users.controller.js' | ||
|
||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These changes are actually what fixed the bug for this route and the health route above, as we had switched to The switch from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for fixing, I completely miss understood |
||
} | ||
get user() { | ||
return new UsersController() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,8 @@ describe('Home Page', () => { | |
beforeEach(() => { | ||
cy.visit(URL) | ||
}) | ||
it('renders a main section', () => { | ||
cy.get('main').should('exist') | ||
|
||
it('renders a main section that successfully interacts with the API', () => { | ||
cy.get('.backend-healthy main').should('exist') | ||
Comment on lines
+8
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ development: | |
PROXIED_API_TOKEN: '' | ||
|
||
# === FE === | ||
REACT_APP_API_URL: http://localhost:8080 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
API_URL: http://localhost:8080/api/v1 | ||
FE_PORT: 3000 | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,14 @@ | ||
import React from 'react' | ||
import App from './App' | ||
import ReactDOM from 'react-dom' | ||
import { api } from '../services/api' | ||
|
||
ReactDOM.render(<App />, document.getElementById('root')) | ||
const root = document.getElementById('root') | ||
|
||
api.get('/api/v1/health').then((response) => { | ||
if (response.healthy) { | ||
root.classList.add('backend-healthy') | ||
} | ||
}) | ||
|
||
ReactDOM.render(<App />, root) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed these files crop up in the diff when running cypress locally