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 test for 'clean' argument passed to 'start_polling()' #2002

Merged
merged 37 commits into from
Jun 23, 2020

Conversation

ikkemaniac
Copy link
Contributor

@ikkemaniac ikkemaniac commented Jun 17, 2020

Looking for feedback, first attempt at this.

Kinda feels wrong to raise an error to jump out of the 'update' def when there is success.

Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

Hi. Thanks for the PR.
TBH, I don't really understand, what's going on. IISC, What we would want to test is

  • On the first call of get_updates(), that returns a list of updates with ids
  • Make sure that for every such id i, get_updates() is called with offset=i

I don't really get, how self.update_id achieves that and what the "infinity loop protection" does … Could you try to make the code more intuitive to read? Descriptive namings and precise comments are helpful for that ;)

tests/test_updater.py Outdated Show resolved Hide resolved
tests/test_updater.py Outdated Show resolved Hide resolved
tests/test_updater.py Outdated Show resolved Hide resolved
tests/test_updater.py Outdated Show resolved Hide resolved
tests/test_updater.py Outdated Show resolved Hide resolved
tests/test_updater.py Outdated Show resolved Hide resolved
tests/test_updater.py Outdated Show resolved Hide resolved
tests/test_updater.py Outdated Show resolved Hide resolved
tests/test_updater.py Outdated Show resolved Hide resolved
tests/test_updater.py Outdated Show resolved Hide resolved
@ikkemaniac
Copy link
Contributor Author

ikkemaniac commented Jun 20, 2020

Hi. Thanks for the PR.
TBH, I don't really understand, what's going on. IISC, What we would want to test is

Trying to test clean. Clean will cause all updates to be ignored. It does that by getting all updates and than calling get_updates() with the highest update_id. This results in all updates with a lower id to be discarded.

* On the first call of `get_updates()`, that returns a list of updates with ids

I mocked get_updates() to return a list of fake updates when called the 1 first time. The second time it's called we will pass an in update_id from the bootstrap ____clean function. We want to know what id that is and verify it's the 'highest'.

* Make sure that for every such id `i`, `get_updates()` is called with `offset=i`

bootstrap___clean doesn't behave like this. It just assumes that the updates are chronological and takes update_id from the last item in the list.

I don't really get, how self.update_id achieves that and what the "infinity loop protection" does

My logic is that if we pass 3 id's, and we know the highest id = 3 than we can validate clean by checking which update_id it will call get_updates() with the second time.

inf loop protection is something left over from debugging

If get_updates() behaviour changes, it won't be caught with this test. if we want to test for that we shouldn't mock get_updates().

… Could you try to make the code more intuitive to read? Descriptive namings and precise comments are helpful for that ;)

10-4

Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

Thanks for clarifying! Got it now and the test is looking much better :)
Two minor comments, though.

tests/test_updater.py Outdated Show resolved Hide resolved
tests/test_updater.py Show resolved Hide resolved
@Bibo-Joshi Bibo-Joshi merged commit e603181 into python-telegram-bot:master Jun 23, 2020
@Bibo-Joshi
Copy link
Member

Thank you for your contribution :)

@ikkemaniac ikkemaniac deleted the polling-clean-test branch June 24, 2020 21:49
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants