-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[2.7] Fix tests #17582
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[2.7] Fix tests #17582
Conversation
paradajozsef
commented
Jan 27, 2016
Q | A |
---|---|
Bug fix? | yes |
New feature? | no |
BC breaks? | no |
Deprecations? | no |
Tests pass? | Let's see Travis results |
Fixed tickets | - |
License | MIT |
Doc PR | - |
Other tests are failing too. (They didn't show until this was fixed) In progress.. Status: Needs Work |
@@ -548,7 +548,7 @@ private function assertUrlPackage(ContainerBuilder $container, DefinitionDecorat | |||
private function assertVersionStrategy(ContainerBuilder $container, Reference $reference, $version, $format) | |||
{ | |||
$versionStrategy = $container->getDefinition($reference); | |||
if (null === $version) { | |||
if (null === $version || !$versionStrategy instanceof DefinitionDecorator) { |
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 is $versionStrategy
allowed to be an instance of DefinitionDecorator
while $version
is null
?
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.
You are right, this fix is wrong.
@xabbuh Could you take a look please? Only 5.3 failes now on Travis but for some other reason, not related to tests: Status: Needs Review |
@@ -21,7 +21,7 @@ class LegacyApcUniversalClassLoaderTest extends \PHPUnit_Framework_TestCase | |||
protected function setUp() | |||
{ | |||
if (ini_get('apc.enabled') && ini_get('apc.enable_cli')) { | |||
apcu_clear_cache('user'); | |||
apcu_clear_cache(); | |||
} else { | |||
$this->markTestSkipped('APC is not enabled.'); |
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.
APCu is not enabled.
?
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.
if (null === $version) { | ||
$this->assertEquals('assets.empty_version_strategy', (string) $reference); | ||
} else { | ||
if ($version && $versionStrategy instanceof DefinitionDecorator) { |
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.
if ($version)
is useless
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.
removed, thx
After #17605 merged, this PR fixes the remaining failing tests. |
Thank you @paradajozsef. |
This PR was squashed before being merged into the 2.7 branch (closes #17582). Discussion ---------- [2.7] Fix tests | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | Let's see Travis results | Fixed tickets | - | License | MIT | Doc PR | - Commits ------- c8e1384 [2.7] Fix tests