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

Added unit tests for better code coverage (#368) #431

Merged
merged 15 commits into from
Jan 17, 2018

Conversation

dilraj45
Copy link
Contributor

@dilraj45 dilraj45 commented Jan 10, 2018

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream master branch.
  • I have added necessary documentation (if appropriate)

Changes proposed in this pull request:

  • Unit test for server.py

Screenshot of current coverage report
coverage

@Remorax
Copy link
Contributor

Remorax commented Jan 10, 2018

Please resolve conflicts 😄

@dilraj45 dilraj45 force-pushed the development branch 5 times, most recently from c502cb7 to 5e563d9 Compare January 13, 2018 18:43
.travis.yml Outdated
@@ -10,8 +10,8 @@ install:
before_script:
- flake8 . --count --max-complexity=15 --show-source --statistics
script:
- python app/server.py > /dev/null &
- pytest --capture=sys --showlocals
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove —showlocals? I find that option quite helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add the showlocals option. I guess, I overlooked this while fixing conflicts, apologies for that

Added dependency on mock in requirement.txt

Updated server.py to use absolute imports

Changed the invocation of server.py to 'python -m app.server' in .travis.yml
Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Nice work!!

Remorax pushed a commit to Remorax/query-server that referenced this pull request Jan 14, 2018
@dilraj45 dilraj45 changed the title Added unit test for server.py (#368) Added unit tests for better code coverage (#368) Jan 15, 2018
@codecov
Copy link

codecov bot commented Jan 15, 2018

Codecov Report

Merging #431 into master will increase coverage by 43.8%.
The diff coverage is 96.4%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #431      +/-   ##
==========================================
+ Coverage   47.26%   91.06%   +43.8%     
==========================================
  Files          17       28      +11     
  Lines         402      627     +225     
==========================================
+ Hits          190      571     +381     
+ Misses        212       56     -156
Impacted Files Coverage Δ
test/test_ask.py 100% <100%> (ø)
test/test_yahoo.py 100% <100%> (ø)
test/test_parsijoo.py 100% <100%> (ø)
test/test_baidu.py 100% <100%> (ø)
test/test_mojeek.py 100% <100%> (ø)
test/test_duckduckgo.py 100% <100%> (ø)
test/test_quora.py 100% <100%> (ø)
test/test_bing.py 100% <100%> (ø)
test/test_google.py 100% <100%> (ø)
test/test_youtube.py 100% <100%> (ø)
... and 27 more

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 fac4abf...0ba447f. Read the comment docs.

Added dependency on mock in requirement.txt

Updated server.py to use absolute imports

Changed the invocation of server.py to 'python -m app.server' in .travis.yml
mojeek.py. parsijoo.py, quora.py, yahoo.py, youtube.py (fossasia#368)
Copy link
Member

@ParthS007 ParthS007 left a comment

Choose a reason for hiding this comment

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

LGTM ! Good work @dilraj45 .

@ParthS007
Copy link
Member

@bhaveshAn @vaibhavsingh97 Please review

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Instead of raising AssertionErrors, please adopt the bandit.yml change from #385 and then go back to using assert statements. The tests will be easier to understand that way.

Copy link
Member

@realslimshanky realslimshanky left a comment

Choose a reason for hiding this comment

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

Please squash your commits.

@ParthS007 ParthS007 merged commit 8d319e7 into fossasia:master Jan 17, 2018
@dilraj45 dilraj45 deleted the development branch January 31, 2018 18:07
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.

5 participants