[Captcha] Covering the CheckUserLoginBackendObserver by Unit Test #19175
Conversation
magento-engcom-team
added
Partner: Atwix
|
Hi @eduard13. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
There was a problem hiding this comment.
@eduard13 thanks, I didn't meet anything really bad during review, just some polishing up is needed. I believe you'll be able to get used soon, just didn't pay attention to some nuances before (and you're not alone - there is a HUGE amount of really fragile unit tests written in Magento 2).
Please consider using Prophecy with your next unit test ;)
| * @param bool $isCorrect | ||
| * @param int $invokedTimes | ||
| * @return void | ||
| * @throws AuthenticationException |
There was a problem hiding this comment.
What? Test throwing exception?
| $this->helperMock->expects($this->once()) | ||
| ->method('getCaptcha') | ||
| ->with($formId) | ||
| ->willReturn($captcha); |
There was a problem hiding this comment.
Most of this assignments would be much more readable as one-liners.
| $captcha->expects($this->once())->method('isRequired') | ||
| ->with($login) | ||
| ->willReturn($isRequired); | ||
| $captcha->expects($this->exactly($invokedTimes)) |
There was a problem hiding this comment.
Such assertion does not test observer behavior, please remove it.
| $observerMock->expects($this->any()) | ||
| ->method('getEvent') | ||
| ->willReturn($eventMock); | ||
| $captcha->expects($this->once())->method('isRequired') |
There was a problem hiding this comment.
$this->once() does not test observer behavior, remove it.
| ->method('isCorrect') | ||
| ->with($captchaValue) | ||
| ->willReturn($isCorrect); | ||
| $this->helperMock->expects($this->once()) |
There was a problem hiding this comment.
$this->once() does not test observer behavior, remove it.
| ->with($this->requestMock, $formId) | ||
| ->willReturn($captchaValue); | ||
|
|
||
| $this->expectException(AuthenticationException::class); |
| $observerMock->expects($this->any()) | ||
| ->method('getEvent') | ||
| ->willReturn($eventMock); | ||
| $captcha->expects($this->once())->method('isRequired') |
There was a problem hiding this comment.
Too strict, irrelevant assertion.
| $captcha->expects($this->once())->method('isRequired') | ||
| ->with($login) | ||
| ->willReturn(true); | ||
| $captcha->expects($this->exactly(1)) |
There was a problem hiding this comment.
Too strict, irrelevant assertion.
| ->method('isCorrect') | ||
| ->with($captchaValue) | ||
| ->willReturn(false); | ||
| $this->helperMock->expects($this->once()) |
There was a problem hiding this comment.
Too strict, irrelevant assertion.
| ->with($formId) | ||
| ->willReturn($captcha); | ||
|
|
||
| $this->captchaStringResolverMock->expects($this->exactly(1)) |
There was a problem hiding this comment.
Too strict, irrelevant assertion.
There was a problem hiding this comment.
Hi @orlangur thank you for all the suggestions. By 'Too strict', do you mean that we should try to avoid the expects strictness in some cases, and use it only when is really needed?
There was a problem hiding this comment.
@eduard13 yes, such checks control execution flow rather than code behavior. When method changes, they may break even when method still works correctly. It is better in each scenario assert only what is really essential for a particular case.
|
Hi @orlangur, could you please review the latest changes? |
|
Hi @orlangur, thank you for the review. |
|
Hi @eduard13. Thank you for your contribution. |
Description
This PR adds missing unit tests for the following class:
\Magento\Captcha\Observer\CheckUserLoginBackendObserverFixed Issues (if relevant)
N/A
Manual testing scenarios
N/A
Contribution checklist