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

Improve test coverage #88

Merged
merged 5 commits into from
Jan 31, 2018
Merged

Improve test coverage #88

merged 5 commits into from
Jan 31, 2018

Conversation

YJDave
Copy link

@YJDave YJDave commented Jan 22, 2018

Try to improving code coverage by adding more tests for functions.

@codecov-io
Copy link

codecov-io commented Jan 22, 2018

Codecov Report

Merging #88 into master will increase coverage by 5.36%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #88      +/-   ##
==========================================
+ Coverage   56.29%   61.66%   +5.36%     
==========================================
  Files           1        1              
  Lines         254      253       -1     
==========================================
+ Hits          143      156      +13     
+ Misses        111       97      -14
Impacted Files Coverage Δ
loklak.py 61.66% <100%> (+5.36%) ⬆️

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 d35dd21...7a611d1. Read the comment docs.

@singhpratyush
Copy link
Member

@YJDave: It seems like there is some issue with rebasing this branch. Please follow the best practices and keep your branch in sync with upstream/master.

@YJDave
Copy link
Author

YJDave commented Jan 23, 2018

@singhpratyush Thanks for reviewing!
Updated!

@YJDave YJDave force-pushed the tests branch 2 times, most recently from e405a6e to 7fd419c Compare January 25, 2018 09:49
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

def test_aggregations(self):
"""Test aggregations."""
result = self.loklak.aggregations('fossasia', '2017-01-10',
'2018-01-10', 10)
data = result.json()
self.assertEqual(result.status_code, 200)
self.assertTrue('aggregations' in data)

result = self.loklak.aggregations()
self.assertEqual(result, '{"error": "No Query string has been given to run query for aggregations"}')
# self.assertTrue('hashtags' in data['aggregations'])
Copy link
Member

Choose a reason for hiding this comment

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

Have you found solution to include fields? Please see this #75 (comment) for more info

Copy link
Author

Choose a reason for hiding this comment

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

Not yet, but I will check it out soon.

Copy link
Member

@daminisatya daminisatya left a comment

Choose a reason for hiding this comment

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

Squash commits!

@YJDave
Copy link
Author

YJDave commented Jan 30, 2018

@daminisatya These all commits have different features/functions/code added, so that in future if somethings goes wrong we can revert the particular commit. Same goes to PR #90
Is this that we should only have one commit in one PR? Can you elaborate more why I should squash them in one? Do you want to me to have them in separate PR with different issues?
Thanks for reviewing :)

@YJDave YJDave force-pushed the tests branch 2 times, most recently from da7dd2a to 224a425 Compare January 30, 2018 10:23
@YJDave
Copy link
Author

YJDave commented Jan 30, 2018

@daminisatya Thanks for your advice. I have updated PR as we talked on channel.

Add tests for getBaseUrl() method:
- To improve the coverage of main code base.
- To test functionality of getBaseUrl() method using
  assertion.
This commit get the 100% coverage of status() method by
passing wrong url to method, to be sure that method does
handle wrong url return proper result.
This commit add tests for:
- Getting user with specific no of followers and following
  by passing no of followers and following as a query params.
- Passing null argument to method, which should return proper
  error message.
This commits adds:
- Proper response message on null query arguments.
- Tests for aggregations() method with null query
  arguments.
Add tests for:
- Getting result in specific time interval by passing
  date/hour argument to method.
- Passing null query arguments for method which should
  return error with proper warning.
@YJDave
Copy link
Author

YJDave commented Jan 31, 2018

@daminisatya ^^

@daminisatya daminisatya merged commit 2eda64e into loklak:master Jan 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants