Skip to content

Conversation

@tobiaswicker
Copy link
Contributor

@tobiaswicker tobiaswicker commented Nov 28, 2019

Fix for issue #1652

@tobiaswicker
Copy link
Contributor Author

Forced push was necessary to trigger the checks to run again, because one failed for unrelated reasons.

@Bibo-Joshi Bibo-Joshi added this to the 12.5 milestone Nov 29, 2019
@Eldinnie
Copy link
Member

`Thanks for making this fix.
Can we explore adding a test to make sure this (keeps) work(ing/s) as intended?

@Eldinnie Eldinnie modified the milestones: 12.5, 12.3 Dec 10, 2019
@tobiaswicker
Copy link
Contributor Author

@Eldinnie PTB does have tests for the ConversationHandler and there is even a test_conversation_handler_timeout_state() test case that specifically tests the ConversationHandler.TIMEOUT state. I think the issue you are talking about, is that those tests don't use_context, right?

So would you like to see the existing tests migrated to use context or rather have separate tests? I'd opt for the latter for backward compatibility for now.

Copy link
Member

@tobiaswicker Switching all the tests to contexts is actually an issue, #1509

@tobiaswicker
Copy link
Contributor Author

@Poolitzer OK, thanks!

@Eldinnie does the mentioned issue satisfy your request for a test or have you had a test in mind that wouldn't be covered by migrating any of the existing tests?

Copy link
Member

@tobiaswicker I am very sure that solving that issue will satisfy his testing needs

Copy link
Member

@Poolitzer Note, that #1509 is currently assigned only to the v13,0 milestone, which will probably not be closed too soon.

Copy link
Member

@Bibo-Joshi I mean, we are talking about a test for a callback here. I dont think it will break soon enough that we will have a problem with adding this test later, do you?

Copy link
Member

@Poolitzer I guess not. Just wanted to mention it ;)

Copy link
Member

@Bibo-Joshi Fair enough

Copy link
Member

@tobiaswicker IMO the new PR should be paired with a test. One that would fail without this fix, and will pass with this fix.

Bibo-Joshi added a commit that referenced this pull request Jan 10, 2020
@tsnoam tsnoam merged commit 940b42e into python-telegram-bot:master Jan 11, 2020
@tsnoam
Copy link
Member

tsnoam commented Jan 11, 2020

@tobiaswicker thankyou for your contribution

tsnoam pushed a commit that referenced this pull request Jan 11, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Aug 19, 2020
@Bibo-Joshi Bibo-Joshi added 🔌 bug pr description: bug and removed bug 🐛 labels Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

🔌 bug pr description: bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants