Skip to content

[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

Closed
Closed

Conversation

paradajozsef
Copy link
Contributor

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 -

@paradajozsef paradajozsef changed the title [FrameworkBundle] Fix frameworkextension test [WIP][FrameworkBundle] Fix FrameworkBundle tests Jan 27, 2016
@paradajozsef
Copy link
Contributor Author

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) {
Copy link
Member

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?

Copy link
Contributor Author

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.

@paradajozsef
Copy link
Contributor Author

@xabbuh Could you take a look please? Only 5.3 failes now on Travis but for some other reason, not related to tests:
https://travis-ci.org/symfony/symfony/jobs/105325452

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.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APCu is not enabled.?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, then the message should be changed here and here too. WDYT?

(And actually idk why we check here apc ini settings when apcu_clear_cache used)

if (null === $version) {
$this->assertEquals('assets.empty_version_strategy', (string) $reference);
} else {
if ($version && $versionStrategy instanceof DefinitionDecorator) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if ($version) is useless

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed, thx

@paradajozsef paradajozsef changed the title [WIP][FrameworkBundle] Fix FrameworkBundle tests [2.7] Fix tests Jan 30, 2016
@paradajozsef
Copy link
Contributor Author

After #17605 merged, this PR fixes the remaining failing tests.

@fabpot
Copy link
Member

fabpot commented Jan 30, 2016

Thank you @paradajozsef.

fabpot added a commit that referenced this pull request Jan 30, 2016
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
@xabbuh xabbuh closed this Jan 30, 2016
@paradajozsef paradajozsef deleted the fix-fwextension-test-2.7 branch January 13, 2017 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants