[Form] do not use mocks in tests when not necessary #46030
Conversation
xabbuh
commented
Apr 13, 2022
| Q | A |
|---|---|
| Branch? | 4.4 |
| Bug fix? | no |
| New feature? | no |
| Deprecations? | no |
| Tickets | |
| License | MIT |
| Doc PR |
1f76255 to
e807cbc
Compare
| $this->assertSame($list, $this->factory->createListFromChoices([])); | ||
| $this->assertSame($list, $this->factory->createListFromChoices([])); | ||
| $this->assertEquals(new ArrayChoiceList([]), $this->factory->createListFromChoices([])); | ||
| $this->assertEquals(new ArrayChoiceList([]), $this->factory->createListFromChoices([])); |
There was a problem hiding this comment.
The caching factory should ensure the same instance of list is returned, this test does not assert the same thing anymore and allows regressions.
There was a problem hiding this comment.
I would revert the changes in this test class.
There was a problem hiding this comment.
I have reworked the test to always check if the created lists are (not) the same explicitly.
src/Symfony/Component/Form/Tests/Fixtures/DummyFormTypeGuesser.php
Outdated
Show resolved
Hide resolved
|
This a great move, thanks ans good job! |
9b0d1c4 to
07dd74b
Compare
c3dadc8 to
0e4feb9
Compare
|
I have finished the changes. The PR is ready to be reviewed. Status: Needs Review |
545ea5d to
0c75820
Compare
| $this->dataCollector->buildPreliminaryFormTree($form); | ||
| $this->dataCollector->collectSubmittedData($form); | ||
|
|
||
| $this->assertNotSame( |
There was a problem hiding this comment.
Is there something we can expect here? assertNotSame with two arrays could mean anything and doesn't seem to bring any value.
There was a problem hiding this comment.
I just wanted to make sure, that there actually is any collected data so that calling reset() afterwards actually has an effect and that the following assertion isn't passing by luck.
There was a problem hiding this comment.
OK, so what about something like assertGreaterThan(0, $this->dataCollector->getData()['forms'])?
There was a problem hiding this comment.
Hm yeah, might make sense. I am going to send a follow-up PR later if you haven't done that in the meantime. :)
…buh) This PR was merged into the 5.4 branch. Discussion ---------- [Form] do not use mocks in tests when not necessary | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | continuation of #46030 for 5.4+ Commits ------- 488a262 do not use mocks in tests when not necessary
This PR was merged into the 4.4 branch. Discussion ---------- [Form] improve an assertion | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | #46030 (comment) | License | MIT | Doc PR | Commits ------- a16b56d improve an assertion