Skip to content
This repository was archived by the owner on Oct 21, 2020. It is now read-only.

Just Jesting #69

Merged
merged 6 commits into from
Apr 26, 2018
Merged

Just Jesting #69

merged 6 commits into from
Apr 26, 2018

Conversation

ojongerius
Copy link
Contributor

@ojongerius ojongerius commented Apr 25, 2018

This currently is mostly scaffolding: it hooks up Jest, and sets up a config that spins up a MongoDB server in memory -according to docs that should take about 7MB or RAM and runs tests against it. Saves us from wiring up Docker and configuring services in Travis.

There is an issue with running these tests in parallel, see jestjs/jest#5731 for now we can run tests serially, which is the recommended mode for CI anyway 🤷‍♂️ .

The only test I've added for now is test/user.test.js, which creates a user using mongoose, and tests if it can retrieve it using graphql. As that endpoint has 2 PRs open and I don't want to get in their way, I won't try to get this merged yet. However, I'd love for us to start having a healthy culture of tests in place sooner rather than later 😺

@Bouncey, @raisedadead I've added you as reviewers but mind the WiP label, it's there for a reason .

Replaces #66

@ojongerius ojongerius added the WiP Work in Progress, not ready for QA/merge label Apr 25, 2018
@ojongerius ojongerius mentioned this pull request Apr 25, 2018
@ojongerius ojongerius changed the title feat(testing): adds Jest config and one small test Just Jesting Apr 25, 2018
@Bouncey
Copy link
Member

Bouncey commented Apr 25, 2018

I think this should be merged as soon as possible. Then any new queries/mutations/utility-functions can be written with tests

Copy link
Member

@Bouncey Bouncey left a comment

Choose a reason for hiding this comment

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

Only one suggestion/question re: the schema.

Apart from that, I have no idea what I am talking about with classes, the only thing I have ever done with super is pass it props in react. And I only do that because I have seen other people to it. So, totally not qualified to talk about them in this instance.

EDIT: After re-reading this, I just giggled at 'instance'. /r/ProgrammerHumor

const { makeExecutableSchema } = require('graphql-tools');
import typeDefs from '../graphql/typeDefs';
import resolvers from '../graphql/resolvers';
const graphqlSchema = makeExecutableSchema({

This comment was marked as off-topic.

This comment was marked as off-topic.

@ojongerius
Copy link
Contributor Author

Thanks @Bouncey, I'll see if I can address your issue tomorrow, happy to hear you are receptive to getting these tests in earlier sooner than later.

@ojongerius
Copy link
Contributor Author

So that's done. Test succeeds, now I just need to make linting pass, and resolve conflicts. @Bouncey @raisedadead good to merge after those chores have been taken care of?

Copy link
Member

@raisedadead raisedadead left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Is this ready for QA?

@ojongerius ojongerius removed the WiP Work in Progress, not ready for QA/merge label Apr 26, 2018
@ojongerius ojongerius merged commit 9840f99 into freeCodeCamp:staging Apr 26, 2018
@ojongerius ojongerius deleted the just_jestingk branch April 26, 2018 02:37
@Bouncey
Copy link
Member

Bouncey commented Apr 26, 2018

Hi @ojongerius. We shouldn't be merging our own PRs at this time.

We could have combined the multiple no-undefined comments into a single comments at the top of the test file. I can pick this up in a separate PR though, no need to revert this.

@raisedadead raisedadead mentioned this pull request Apr 26, 2018
@ojongerius
Copy link
Contributor Author

Sure! Got a bit impatient 😃

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.

3 participants