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

More assertions #55

Merged
merged 2 commits into from
Nov 13, 2017
Merged

More assertions #55

merged 2 commits into from
Nov 13, 2017

Conversation

Srokap
Copy link
Contributor

@Srokap Srokap commented Nov 11, 2017

Refs: #53

Still WIP, but wanted to make early PR to keep track of it against the issue.

I added asymmetric matchers to make tests a bit more readable. Spotted some inconsistencies in vehicles and did most of assertions on them. You can see the comments.

@Srokap Srokap changed the title Refs #53: More assertions More assertions Nov 11, 2017
@codecov-io
Copy link

codecov-io commented Nov 11, 2017

Codecov Report

Merging #55 into master will increase coverage by 0.66%.
The diff coverage is 94.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #55      +/-   ##
==========================================
+ Coverage   90.04%   90.71%   +0.66%     
==========================================
  Files           8        9       +1     
  Lines         201      237      +36     
  Branches       28       29       +1     
==========================================
+ Hits          181      215      +34     
- Misses         20       22       +2
Impacted Files Coverage Δ
test/utilities/custom-asymmetric-matchers.js 100% <100%> (ø)
routes/v1-launches.js 84.61% <60%> (-2.89%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88075bf...4b5cb4d. Read the comment docs.

res.status(404)
return res.json(error)
}
res.json(doc[0])
Copy link
Contributor Author

@Srokap Srokap Nov 11, 2017

Choose a reason for hiding this comment

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

This is significant if anyone uses it already. Probably makes more sense to return single item instead of array with single item.

@jakewmeyer
Copy link
Member

Looks good, looked through the other repos and didn't see anyone using the latest endpoint for anything live currently. I'll post a heads up about the change for those working on other projects 👍

@jakewmeyer jakewmeyer merged commit f3907f2 into r-spacex:master Nov 13, 2017
@Srokap
Copy link
Contributor Author

Srokap commented Nov 13, 2017

I probably should split the latest endpoint change from this one. This is mostly about data that needs to get fixed in mongo and discussing options. Wanted to make some summary with current requests, but wasn't feeling well over the weekend. Feel free to have a look at lines that are commented out. Most are just fields that are missing in some entries and are present others. One field was inconsistent type IIRC, number/object, but don't remember off top of my head, hopefully I mentioned it in comment.

Edit: Just realized it's already merged, nvm then :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants