Skip to content

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

@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.

@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.

2 participants