Skip to content
This repository has been archived by the owner on Jun 9, 2023. It is now read-only.

add serverside testing #139

Merged
merged 15 commits into from
Dec 11, 2019
Merged

Conversation

timmyichen
Copy link
Contributor

@timmyichen timmyichen commented Oct 24, 2019

  • I have read Chapter's contributing guidelines.
  • My pull request has a descriptive title (not a vague title like Update README.md).
  • My pull request targets the master branch of Chapter.

Adds serverside testing:

  • Adds custom App class with all the setup/takedown for tests
  • Adds tests for the three routes defined in exampleRouter.ts
  • Pulls out jest config from package.json into external jest.config.js since it was getting quite long
  • Uses docker-compose for testing since we will need it once the database is integrated

I'm not sure what best practices are for this so let me know how this pattern looks and any feedback you might have. Thanks!

TODOS:

  • fix client tests
  • update docs

@timmyichen timmyichen changed the title Dev/router tests add serverside testing [WIP] Oct 24, 2019
@vaibhavsingh97
Copy link
Member

The PR looks good to me

@timmyichen
Copy link
Contributor Author

Gonna wait to rebase with #146 before continuing

@vaibhavsingh97
Copy link
Member

vaibhavsingh97 commented Nov 18, 2019

@timmyichen I think you forgot about this PR 😛, pinging you so that we can merge once you update this PR 😃

@timmyichen
Copy link
Contributor Author

@vaibhavsingh97 still waiting on #146 🙃

@QuincyLarson
Copy link
Contributor

In summary: this is blocked by NodeMailer. Once we include NodeMailer we will be able to return to QA'ing this.

@timmyichen timmyichen changed the title add serverside testing [WIP] add serverside testing Dec 6, 2019
@timmyichen timmyichen marked this pull request as ready for review December 6, 2019 11:41
Copy link
Member

@vaibhavsingh97 vaibhavsingh97 left a comment

Choose a reason for hiding this comment

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

LGTM 🔥 , just a minor concern

@@ -19,6 +19,10 @@ services:
- DB_PASSWORD=password
- DB_NAME=chapter
- DB_URL=db
- CHAPTER_EMAIL=ourEmail@placeholder.place
Copy link
Member

Choose a reason for hiding this comment

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

Env variables will grow in the future, shouldn't we maintain separate .env

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for the most part, there's no functional difference between having a long list of env variables in the docker-compose file and a long list of env variables in .env. With a separate .env file, you are requiring people to copy/rename a file before running it whereas this will be set up when you run docker-compose up.

Copy link
Member

Choose a reason for hiding this comment

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

For the sake of simplicity I think that for now we should stick with env vars in docker-compose, but maybe later when we have a lot of them we could extract them.

@vaibhavsingh97
Copy link
Member

@timmyichen can you please resolve merge conflicts

@timmyichen
Copy link
Contributor Author

sorry about the delay - done!

@Zeko369 Zeko369 merged commit b77d3a0 into freeCodeCamp:master Dec 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants