Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[PhpUnitBridge] deprecate the testLegacy test name prefix#21140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xabbuh commentedJan 2, 2017
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | yes |
| Tests pass? | yes |
| Fixed tickets | #18755 |
| License | MIT |
| Doc PR |
| } | ||
| } | ||
| if ($testinstanceof \PHPUnit_Framework_TestCase &&0 ===strpos($test->getName(),'testLegacy') &&empty($test->getTestResultObject()->warningCount())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
when the annotation is in place, we should not trigger the warning. What about the other cases?
From DeprecationErrorHandler:
}elseif (0 ===strpos($method,'testLegacy') ||0 ===strpos($method,'provideLegacy') ||0 ===strpos($method,'getLegacy') ||strpos($class,'\Legacy')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Adding the warning based on class/method names does work. I am looking into how to do that (if possible) based on the data transformer.
xabbuh commentedJan 2, 2017
Status: Needs work |
| publicfunctionaddWarning(\PHPUnit_Framework_Test$test,\PHPUnit_Framework_Warning$e,$time) | ||
| { | ||
| if ($testinstanceof \PHPUnit_Framework_TestCase) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why do we need this check here, we already typehint the parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The type hint is for\PHPUnit_Framework_Test, but not\PHPUnit_Framework_TestCase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
damn, you are right 👍
xabbuh commentedJan 3, 2017
The Status: Needs review |
xabbuh commentedJan 3, 2017
After talking to@nicolas-grekas the warnings will now only be triggered if the |
693cf86 to2448fbcCompare
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 with minor comments
| } | ||
| if ($testinstanceof \PHPUnit_Framework_TestCase &&0 ===strpos($test->getName(),'testLegacy') && !isset($this->testsWithWarnings[$test->getName()]) && !in_array('legacy',$groups,true)) { | ||
| $test->getTestResultObject()->addWarning($test,new \PHPUnit_Framework_Warning('Using the testLegacy prefix to mark tests as legacy is deprecated since version 3.3 and will be removed in 4.0. Use the "@group legacy" notation instead to add the test to the legacy group.'),$time); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
quotes around "testLegacy"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
quotes added in both places
| } | ||
| if ($testinstanceof \PHPUnit_Framework_TestCase &&strpos($className,'\Legacy') && !isset($this->testsWithWarnings[$test->getName()]) && !in_array('legacy',$classGroups,true)) { | ||
| $test->getTestResultObject()->addWarning($test,new \PHPUnit_Framework_Warning('Using the Legacy prefix to mark all tests of a class as legacy is deprecated since version 3.3 and will be removed in 4.0. Use the "@group legacy" notation instead to add the test to the legacy group.'),$time); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
quotes around "Legacy"
nicolas-grekas commentedJan 6, 2017
Thank you@xabbuh. |
…fix (xabbuh)This PR was merged into the 3.3-dev branch.Discussion----------[PhpUnitBridge] deprecate the testLegacy test name prefix| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets |#18755| License | MIT| Doc PR |Commits-------21b72dd deprecate the Legacy/testLegacy test name prefix
…, javiereguiluz)This PR was merged into the master branch.Discussion----------Do not document deprecated legacy test mechanismsseesymfony/symfony#21140Commits-------c2c680a Fixed a minor typo6f546aa do not document deprecated legacy test mechanisms
This PR was merged into the 3.3 branch.Discussion----------[PhpUnitBridge] add changelog entries for#21140| Q | A| ------------- | ---| Branch? | 3.3| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#21140| License | MIT| Doc PR |Commits-------c83c5bc [PhpUnitBridge] add changelog entries for#21140
* 3.3: [Serializer] Remove a useless legacy annotation Fixed extra semi-colon fix docblock position [DependencyInjection] remove unused variable [PhpUnitBridge] add changelog entries for#21140 [DI] Remove dead service_container checks
* 3.4: [Serializer] Remove a useless legacy annotation Fixed extra semi-colon fix docblock position [DependencyInjection] remove unused variable [PhpUnitBridge] add changelog entries for#21140 [DI] Remove dead service_container checks