-
-
Notifications
You must be signed in to change notification settings - Fork 771
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
Writing coverage for marketing model, increasing coverage to 33% #524
Conversation
@owocki hey, I'd like to merge it but it seems like the tests are failing again with same error as before
|
d0b2f62
to
de2cf45
Compare
shoot.. you are right.. 9c82c4f should fix that |
just poked through the new tests.. 💯 thanks so much! |
Yeah, we talked. it's good now 👍
You're welcome 😄, learned a thing or two writing the tests. I think more bounties like this are needed, I communicate some more thoughts later. For now |
did you mean that my commit (9c82c4f) broke the tests? or something else? |
Oh no, some logic was changed before.. I assumed it was intentionally
…On Tue, Mar 6, 2018, 12:24 Kevin Owocki ***@***.***> wrote:
For now expiration_start_work implementation changed so the tests not
pass, I'll look at that at the evening I guess.
did you mean that my commit (hash pasted above in my previous comment)
broke the tests?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#524 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AG5NrFHzvVBt4g5u03XfKSTGpyxP1HkFks5tbmPmgaJpZM4SbdJL>
.
|
de2cf45
to
9801ad6
Compare
Codecov Report
@@ Coverage Diff @@
## master #524 +/- ##
==========================================
+ Coverage 29.11% 33.98% +4.86%
==========================================
Files 88 88
Lines 4880 4882 +2
Branches 571 571
==========================================
+ Hits 1421 1659 +238
+ Misses 3412 3157 -255
- Partials 47 66 +19
Continue to review full report at Codecov.
|
The logic that was changed in 9c82c4f is just that if a comment happens before the 'start work' button is clicked it is disregarded.. Let me know if you have other questions to get the tests working agin |
Yes I solved that. Ok I think that this can be merged 😄, the last thing is about the linter? Which one you are using? |
@leonprou I regularly use |
@owocki I'm all for updating the docs to reflect a standard linting practice. I've been meaning to ask you about your thoughts on: https://stickler-ci.com/ - It's free for OSS. We could simply use their linter without using the |
certainly open to trying it.. hopefully between travis + codecov + stickler its not too overwhelming for new contributors |
9165fc0
to
79623c6
Compare
@leonprou Is this ready to review and test? |
Yes it's ready
There's a problem in master I think, a linting (isort) is failing.
…On Fri, Mar 9, 2018, 21:41 Mark Beacom ***@***.***> wrote:
@leonprou <https://github.com/leonprou> Is this ready to review and test?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#524 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AG5NrCvXAjWkss4qVtpjefnZ43Y4wTD2ks5tctr5gaJpZM4SbdJL>
.
|
@leonprou Yeah, that's fine... It's an isort failure on an import in Reviewing this now. |
"""Define tests for roundup send subscription mails.""" | ||
|
||
def test_handle(self): | ||
"""Test command send subscription mails.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to call Command().handle()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, forgot to test that one 👍
"""Test the marketing util get_or_save_email_subscriber method.""" | ||
assert get_or_save_email_subscriber('emailSubscriber1@gitcoin.co', 'mysource') == 'priv1' | ||
|
||
emailSubscriber1SecondSource = EmailSubscriber.objects.create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you camel_case
this variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually it's unused variable)
'all_earners': {}, | ||
} | ||
def default_ranks(): | ||
return { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
idx_status='submitted', | ||
current_bounty=True | ||
) | ||
bounty_fulfillment = BountyFulfillment.objects.create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove the unused assignment of bounty_fulfillment
here?
assert assemble_leaderboards.ranks['monthly_all']['fred'] == 3 | ||
assert assemble_leaderboards.ranks['yearly_all']['fred'] == 3 | ||
|
||
assert len(assemble_leaderboards.ranks['weekly_fulfilled']) == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opposed to len()
, can you use either assert any(dict) is False
or assert not dict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done,
buy why you don't like len()
?
bounty=bounty | ||
) | ||
|
||
bounty_fulfillment = BountyFulfillment.objects.create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove the unused assignment here?
class TestSyncKeywords(TestCase): | ||
"""Define tests for sync keywords.""" | ||
|
||
def setUp(test): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you switch this to def setUp(self):
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure 👍
def test_get_stat(self): | ||
"""Test the marketing util get_stat method.""" | ||
assert get_stat('mykey') == 2 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add another new line between these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you want to separate the classes by 2 lines, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes @leonprou - Two new lines between module level classes and methods.
import thing
class Example:
"""Some docstring."""
def test(self):
"""Some test."""
return 'test'
def test_again(self):
"""Some test again."""
return 'test again'
class ExampleAgain:
"""Some docstring again."""
def test(self):
"""Some test."""
return 'test'
def some_test():
"""Some test."""
return 'some test'
|
||
def test_should_suppress_notification_email(self): | ||
"""Test the marketing util test_should_suppress_notification_email method.""" | ||
assert should_suppress_notification_email('emailSubscriber1@gitcoin.co') == False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change these boolean assertions to assert func() is False
and assert func() is True
or assert func()
and assert not func()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, thanks 👍
def test_save_get_or_save_email_subscriber_get(self): | ||
"""Test the marketing util get_or_save_email_subscriber method.""" | ||
self.assertIsNotNone(get_or_save_email_subscriber('newemail@gitcoin.co', | ||
'mysource', send_slack_invite=False)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you carry this indent over to hang below (get_or_save
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
never use how to indent it right, but I fixed and flake
is not complaining
Overall, lgtm - Just a few miscellaneous syntax changes. Thanks for the contribution @leonprou ! |
79623c6
to
db889b3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think I made all the changes, also ran flake
myself on all tests files
|
||
def test_should_suppress_notification_email(self): | ||
"""Test the marketing util test_should_suppress_notification_email method.""" | ||
assert should_suppress_notification_email('emailSubscriber1@gitcoin.co') == False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, thanks 👍
"""Test the marketing util get_or_save_email_subscriber method.""" | ||
assert get_or_save_email_subscriber('emailSubscriber1@gitcoin.co', 'mysource') == 'priv1' | ||
|
||
emailSubscriber1SecondSource = EmailSubscriber.objects.create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually it's unused variable)
def test_save_get_or_save_email_subscriber_get(self): | ||
"""Test the marketing util get_or_save_email_subscriber method.""" | ||
self.assertIsNotNone(get_or_save_email_subscriber('newemail@gitcoin.co', | ||
'mysource', send_slack_invite=False)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
never use how to indent it right, but I fixed and flake
is not complaining
assert assemble_leaderboards.ranks['monthly_all']['fred'] == 3 | ||
assert assemble_leaderboards.ranks['yearly_all']['fred'] == 3 | ||
|
||
assert len(assemble_leaderboards.ranks['weekly_fulfilled']) == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done,
buy why you don't like len()
?
"""Define tests for roundup send subscription mails.""" | ||
|
||
def test_handle(self): | ||
"""Test command send subscription mails.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, forgot to test that one 👍
lets get this shipped this week! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Thanks for the contribution and updates @leonprou !
Description
Adding tests to increase code coverage from 28% to 33%. Testing marketing module.
Checklist
Affected core subsystem(s)
marketing
Testing
This PR is increasing test coverage. Only one little refactor was made.
Refers/Fixes
#408