1) Dialog key generation functions refactoring 2) Test to check afterLastStep function #24
Conversation
…p to avoid misunderstanding in private functions naming and to have gen firmula in 1 place for all needs.
… function. Test fails, which prove the wrong place of afterLastStep function call. Ready to do PR of fix (Dialog.php file).
There was a problem hiding this comment.
Hey @dimaodnokoz
Thanks for your work! Generally it looks great, I love the direction. But there are a few things we can improve to make code cleaner. Please take a look to my comments. Thanks again!
PS: BTW, I found a bug when we call getMessage()->id in DialogManager. Due to bad design in the TG SDK v3, getMessage() may return an empty Collection in some cases. We need to fix it also (of course not in this PR)
src/DialogManager.php
Outdated
| * @return non-empty-string | ||
| */ | ||
| private function generateDialogKeySharedBetweenUsers(Update $update): string | ||
| private function generateDialogKey($chatId, $userId = null): string |
There was a problem hiding this comment.
missing type info (both params are int)
src/Dialogs/HelloExampleDialog.php
Outdated
| public function sayHello(Update $update): void | ||
| { | ||
| //Used for test purposes | ||
| if(!isset($this->bot)){return;} |
There was a problem hiding this comment.
this HelloExampleDialog should be sort of showcase for developers, and we should avoid adding such code needed for testing purposes.
if you want to test Dialog-related functionality, please consider either:
- create a new one that can even not call any
- use the same approach as a used to create a fake bot that doesn't use HTTP (see https://github.com/koot-labs/telegram-bot-dialogs/blob/master/tests/Fakes/FakeBot.php)
There was a problem hiding this comment.
Hello, @alies-dev. Thank you for your feedback. So should i cancel this PR and submit another one with fixes?
According your last proposal "use the same approach" - i tried, but failed. Maybe it was because i planned test with sendMessage and did not understand how to emulate its call in fake bot. But as for my last test i do not need this call, so i will try again.
There was a problem hiding this comment.
@dimaodnokoz
pls update code for the existing PR, I think it will be simpler to you.
Also, if you have issues with Faking HTTP requests and responses, I think it will be simpler for your to create a new Dialog implementation like PassiveTestDialog extends Dialog that only listens and doesn't send anything and use it for your tests.
You can also implement afterLastStep in that Dialog and e.g. throw an exception. If exception is not thrown - the test should fail. This way you will avoid adding code like:
public bool $afterLastStepCalled = false;
protected function afterLastStep(Update $update): void
{
$this->afterLastStepCalled = true;
}(it's just an idea, you can find another implementation, incl. public properties that are ok for a TestDialog)
…log added in Dialogs, new exception ItWorks added in tests/Exceptions (here just because it not an exception by meaning, and should be only for test purposes), DialogManagerTest updated.
|
@alies-dev done! plz check. Except a bug about getMessage. |
lptn
left a comment
There was a problem hiding this comment.
Looks great!
I'll remove ItWorks exception if you don't mind and will use \LogicException instead to reduce amount of code. Plus I'll move PassiveTestDialog to simplify onboarding for new comers. I think this class should live in /tests dir
Thank you for your great help!
|
We proved that afterLastStep is not in correct place. So now we need to fix this) Should i wait for your new commit, where you do several things mentioned, above and then commit this fix? Or, maybe, you will fix it yourself? As i understand the call of afterLastStep in Dialog.php/proseed function should be moved to the very end of function right after "++$this->next;" and that's all. |
I did it, it's already on the
Right, somewhere I this place. Ideally we should have more tests to catch all possible cases (e.g. with |
Refactoring of dialog key generation and use of it in DialogManager.php to avoid misunderstanding in private functions naming and to have generation formula in 1 place for all needs. Description of $user_id in Dialog.php also changed to be more informative. All tests passed.
Added test to DialogManager.php to check afterLastStep function call in Dialog.php proceed function. Also updated HelloExampleDialog.php to use it in tests without TG API calls in steps if bot instance NULL (like in tests). Test fails, which prove the wrong place of afterLastStep function call. Ready to do PR of fix in Dialog.php file, plz suggest next steps here.