EZP-28742: Add unit tests to RepositoryForms #307
Conversation
mikadamczyk
force-pushed
the
ezp-28742-add-unit-tests-to-repository-forms
branch
from
f7e7fff to
d206b52
Compare
| $this->assertSame([ContentEditEvents::CONTENT_PREVIEW => ['processPreview', 10]], $previewFormProcessor::getSubscribedEvents()); | ||
| } | ||
|
|
||
| protected function createUntouchableMock(string $class): MockObject |
There was a problem hiding this comment.
What's the thinking behind untouchable mock? Any particular reason why you are mocking a class which should never be used by implementation?
There was a problem hiding this comment.
Yes, if we have situation like in ProcessPreview method, where we want to be sure, that in "positive path" we won't call any method from notificationHandleror translator, this is usecase for "UntouchableMock". E.g. if someone accidentally delete try-catch block.
There was a problem hiding this comment.
Sorry but you are restricting implementation details in the test cases which should NEVER be the case. Right now you are making any refactoring really hard because small change can make tests fail. How not using $translator is a valid assertion from the test standpoint?
| { | ||
| $translator = $this->createMock(TranslatorInterface::class); | ||
| $translator | ||
| ->expects(self::once()) |
There was a problem hiding this comment.
I'm against creating strict mocks that are checking method calls - this is testing implementation detail which should not be in the scope of the test. From my perspective I don't see any point in mocking Translator interface and making sure it was called only once with specific message.
There was a problem hiding this comment.
I think it is important to ensure that we have the right error message, and we do it once. But I would like to hear another opinion @Nattfarinn @ViniTou
There was a problem hiding this comment.
Alright but using "expects only once" affects implementation details which in most cases ends up as failing tests even though the changes were correct. Please test only the outcome which in case of a PreviewFormProcessor might be not that explicit but it would be saving a draft and setting a RedirectResponse with proper URL if I remember correctly.
mikadamczyk
force-pushed
the
ezp-28742-add-unit-tests-to-repository-forms
branch
from
d206b52 to
53dcaa6
Compare
| $contentStruct = $this->generateContentStruct($languageCode, $fieldDefinitionIdentifier, $fieldDataValue); | ||
|
|
||
| $contentDraft = $this->generateContentDraft($contentDraftId, $languageCode); | ||
|
|
There was a problem hiding this comment.
Can you work on code style a bit more? I feel like separating everything with a new line doesn't make sense here.
| private function generateVersionInfo(APIContentInfo $contentInfo): APIVersionInfo | ||
| { | ||
| return new VersionInfo( | ||
| [ |
There was a problem hiding this comment.
For arrays with only one element you can write it inline. No need to expand it as it takes more lines and forces you to scroll more.
mikadamczyk
force-pushed
the
ezp-28742-add-unit-tests-to-repository-forms
branch
from
53dcaa6 to
adf2980
Compare
|
@webhdx are you good now? |
Add unit tests to RepositoryForms
Checklist:
$ composer fix-cs)