Skip to content

added test for 'clean' argument passed to 'start_polling()' - #2002

Merged
Bibo-Joshi merged 37 commits into
python-telegram-bot:masterfrom
ikkemaniac:polling-clean-test
Jun 23, 2020
Merged

added test for 'clean' argument passed to 'start_polling()'#2002
Bibo-Joshi merged 37 commits into
python-telegram-bot:masterfrom
ikkemaniac:polling-clean-test

Conversation

@ikkemaniac

@ikkemaniac ikkemaniac commented Jun 17, 2020

Copy link
Copy Markdown
Contributor

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.

@Bibo-Joshi Bibo-Joshi left a comment

Copy link
Copy Markdown
Member

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 ;)

Comment thread tests/test_updater.py Outdated
Comment thread tests/test_updater.py Outdated
Comment thread tests/test_updater.py Outdated
Comment thread tests/test_updater.py Outdated
Comment thread tests/test_updater.py Outdated
Comment thread tests/test_updater.py Outdated
Comment thread tests/test_updater.py Outdated
Comment thread tests/test_updater.py Outdated
Comment thread tests/test_updater.py Outdated
Comment thread tests/test_updater.py Outdated
@ikkemaniac

ikkemaniac commented Jun 20, 2020

Copy link
Copy Markdown
Contributor Author

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

@Bibo-Joshi Bibo-Joshi left a comment

Copy link
Copy Markdown
Member

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.

Comment thread tests/test_updater.py Outdated
Comment thread tests/test_updater.py
@Bibo-Joshi
Bibo-Joshi merged commit e603181 into python-telegram-bot:master Jun 23, 2020
@Bibo-Joshi

Copy link
Copy Markdown
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.

2 participants