[Security] Readd the correct tests #15932
Conversation
| * | ||
| * For the full copyright and license information, please view the LICENSE | ||
| * file that was distributed with this source code. | ||
| */ |
|
You should not remove tests for the BC layers. Bc layers must be tested too (this is why we have the legacy group) |
|
@stof if you look at the referenced PR, you see that there is a |
|
@wouterj why splitting it into 2 files instead of having them in the same file ? We stopped moving things around into |
|
Btw, I'm quite sure this is what happened here: there was a conflict between branches when merging things, and the conflict was not resolved in the right way, keeping the wrong version of the file... |
|
@stof I find it way more easy to read (both the code and the report in case of failure) to have many tests and multiple test cases instead of having one very big data provider and a couple of tests using this provider. If you think having one class and a data provider is the better solution, I'm open to rewrite the test to use a data provider (the current tests contains more scenarios than the older tests). |
|
@wouterj the legacy tests and the non-legacy ones are not exactly the same. We would have several tests, (for the legacy and non-legacy cases), not a data provider returning the deprecated and non deprecated voters (which would be a pain to handle for the |
|
@stof such specific tests can just be added to the legacy test case: wouterj@df36835#diff-38a5c5ac2cd48e853e59014e2ff23f14R33 Anyway, as it seems you really dislike this approach, I've rewrote the tests to use one very big data provider... |
|
|
||
| public function getTests() | ||
| { | ||
| $object = $this->getMock('AbstractVoterTest_Object'); |
There was a problem hiding this comment.
setUp seems to be run after data providers, so we need to redefine object here
There was a problem hiding this comment.
creating mocks in the data provider is a bad idea. These mocks will not be associated with the test properly (as they are created outside the test), which can lead to weird behaviors.
This PR was merged into the 2.7 branch. Discussion ---------- [Security] Improve AbstractVoter tests Applying the improved tests from #15932 into the oldest possible branch. Merge conflicts from 2.7 into 2.8 caused by this PR do not need to be done carefully, I'll create a new PR for 2.8 updating the tests as soon as these changes are merged up. | Q | A | ------------- | --- | Fixed tickets | - | License | MIT Commits ------- 5ff741d Readd the correct tests
This PR was merged into the 2.7 branch. Discussion ---------- [Security] Improve AbstractVoter tests Applying the improved tests from symfony/symfony#15932 into the oldest possible branch. Merge conflicts from 2.7 into 2.8 caused by this PR do not need to be done carefully, I'll create a new PR for 2.8 updating the tests as soon as these changes are merged up. | Q | A | ------------- | --- | Fixed tickets | - | License | MIT Commits ------- 5ff741d Readd the correct tests
Something went wrong with the merging #15151 , resulting in the wrong tests for the abstract voter.