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

Possibly incorrect test assertions for min_likes and min_retweets #50

Closed
blip-lorist opened this issue Sep 15, 2019 · 2 comments · Fixed by #51
Closed

Possibly incorrect test assertions for min_likes and min_retweets #50

blip-lorist opened this issue Sep 15, 2019 · 2 comments · Fixed by #51

Comments

@blip-lorist
Copy link

blip-lorist commented Sep 15, 2019

Hi there! I was working on a PR to address Issue 49.

However, I noticed some confusing discrepancies between the README and the test assertions, and wanted to clarify first before submitting PRs.

min_likes expectations:

expected = [{"id_str": "21"}, {"id_str": "22"}]

Shouldn't the expected value be [{"id_str": "21"}] ?

According to the docs, it seems like the TweetReader should exclude any tweets with at least one or more retweet.

If you take a look at the actual TweetReader list in that test case by dropping in a breakpoint.

actual = list(TweetReader(FakeReader(tweets), min_likes=1).read())
(Pdb) actual
=> [{'id_str': '21', 'favorite_count': 0}]

min_retweets expectations:
Should this line

expected = [{"id_str": "21"}, {"id_str": "22"}]

be expected = [{'id_str': '21', 'retweet_count': 0}]?
Just like the above example, inspecting the actual list in the test returns: => [{'id_str': '21', 'retweet_count': 0}]

@blip-lorist
Copy link
Author

Since the expected lists don't match the actual lists, both of those tests are false positives. Might want to add

self.assertEqual(len(expected), len(actual))

to avoid false positives.

@koenrh
Copy link
Owner

koenrh commented Sep 21, 2019

Hi @lorainekv,

Apologies for the delayed response.

I think you are right! Feel free to submit the pull request.

koenrh added a commit that referenced this issue Sep 28, 2019
koenrh added a commit that referenced this issue Sep 28, 2019
koenrh added a commit that referenced this issue Sep 28, 2019
koenrh added a commit that referenced this issue Sep 28, 2019
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 a pull request may close this issue.

2 participants