Skip to content
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

fix #707 adding post route for /feeds #729

Merged
merged 1 commit into from Feb 17, 2020

Conversation

Grommers00
Copy link
Contributor

@Grommers00 Grommers00 commented Feb 16, 2020

Issue This PR Addresses

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

This will allow us to connect our front end and back end to post the data to add the feeds to the master list.

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

src/backend/web/routes/feeds.js Show resolved Hide resolved
src/backend/web/routes/feeds.js Show resolved Hide resolved
src/backend/web/routes/feeds.js Outdated Show resolved Hide resolved
src/backend/web/routes/feeds.js Outdated Show resolved Hide resolved
src/backend/web/routes/feeds.js Outdated Show resolved Hide resolved
@humphd
Copy link
Contributor

humphd commented Feb 16, 2020

This also needs some tests.

@humphd humphd changed the title fix #707 adding post route. fix #707 adding post route for /feeds Feb 16, 2020
src/backend/web/routes/feeds.js Outdated Show resolved Hide resolved
src/backend/web/routes/feeds.js Outdated Show resolved Hide resolved
src/backend/web/routes/feeds.js Outdated Show resolved Hide resolved
src/backend/web/routes/feeds.js Outdated Show resolved Hide resolved
src/backend/web/routes/feeds.js Outdated Show resolved Hide resolved
humphd
humphd previously approved these changes Feb 16, 2020
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Code looks good. What are we doing about tests? I think we should get some in so we know this works.

Silvyre
Silvyre previously approved these changes Feb 16, 2020
Copy link
Contributor

@Silvyre Silvyre left a comment

Choose a reason for hiding this comment

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

I like the handling for various status codes!

test/feeds.test.js Outdated Show resolved Hide resolved
Copy link
Member

@manekenpix manekenpix left a comment

Choose a reason for hiding this comment

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

I think you can include your tests in the describe that we already have so you won't have to repeat code (DRY vs WET)

test/feeds.test.js Outdated Show resolved Hide resolved
test/feeds.test.js Outdated Show resolved Hide resolved
test/feeds.test.js Outdated Show resolved Hide resolved
@Grommers00
Copy link
Contributor Author

I think you can include your tests in the describe that we already have so you won't have to repeat code (DRY vs WET)

Not sure about this because a Post route is different than a GET route. Unless you were referring to something different?

@manekenpix
Copy link
Member

Not sure about this because a Post route is different than a GET route. Unless you were referring to something different?

You're right. If you end up not using beforeAll maybe it's better to have your tests in a different set.

@humphd
Copy link
Contributor

humphd commented Feb 16, 2020

Some really good ideas in here. I'll just add/answer a few more:

  • consider each test as an individual "unit", and don't rely on anything external to that test if possible. For example, if you need some data to use, create it in that test. I usually do this by having functions that return data, and call them over and over again in different tests vs. creating variables. Variables are what make tests fail.
  • Tests can and will run in any order. As such, don't have one test depend on state from another (e.g., don't add something in one test, and try to read it in the next test).
  • Tests might not all get run. A dev might run a single test, so don't rely on anything in other tests.
  • If you require the world to be in a certain state for your test, do that in the test itself. If all your tests need that, do beforeAll() or beforeEach(), but prefer doing things in the test if possible so you know they happened in the order you need them to, and nothing else changed their state.

In summary: don't share data between tests, don't rely on one test in another, don't rely on test order.

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Really good. A few small things, and this is R+ from me.

src/backend/web/routes/feeds.js Outdated Show resolved Hide resolved
src/backend/web/routes/feeds.js Outdated Show resolved Hide resolved
test/feeds.test.js Outdated Show resolved Hide resolved
Silvyre
Silvyre previously approved these changes Feb 17, 2020
Copy link
Contributor

@Silvyre Silvyre left a comment

Choose a reason for hiding this comment

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

Congrats on your first Jest test!

humphd
humphd previously requested changes Feb 17, 2020
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Let's add one more thing to your tests, so we know that the feed gets added. I think that's it, the rest looks great.

src/backend/web/routes/feeds.js Show resolved Hide resolved
test/feeds.test.js Show resolved Hide resolved
Copy link
Member

@manekenpix manekenpix left a comment

Choose a reason for hiding this comment

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

👍

@Grommers00 Grommers00 merged commit af58977 into Seneca-CDOT:master Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants