In the original PR the tests were not failling but we should not use the new feature with deps=low until it fetch the branch with the new feature. So this fix the test for the WorkflowExtension in TwigBridge.

carsonbot added Status: Needs Review TwigBridge Deprecation labels

Dec 12, 2017

Copy link

Member

nicolas-grekas commentedDec 12, 2017•
edited
Loading

Should we plan to remove all the new@legacy method? Won't that reduce the test coverage?
Shouldn't we instead upgrade the test suite to use the not-deprecated API (and keep a few legacy calls just to ensure the legacy way keeps working)?
Feels like so to me.

nicolas-grekas added this to the4.1 milestone

Dec 12, 2017

lyrixx added the Workflow label

Dec 12, 2017

Copy link

ContributorAuthor

Simperfit commentedDec 12, 2017 via email•
edited
Loading

we do not plan to remove but to update when we will update the Bridge touse the new features. but yeah I agree if we can update now let’s do it :)

Copy link

Member

chalasr commentedDec 12, 2017

Keeping these legacy tests do not make sense to me either.setUp() should just use the right api based on method existence (the actual code does not even use this part of the api, only the test does).

Copy link

ContributorAuthor

Simperfit commentedDec 12, 2017 via email•
edited
Loading

Oh, maybe we should add a condition like you said and instead of using theannotation use the method from phpunit ?

Copy link

Member

chalasr commentedDec 12, 2017•
edited
Loading

IMO there is no deprecation to assert here.
We want to keep 3.x compatibility, the test should just use the right api i.e.$registry->{method_exists($registry, $_ = 'addWorkflow') ? $_ : 'add'}($workflow, ...);, no possible deprecation as the new api is used if it exists.

Simperfit force-pushed thebugfix/fix-workflow-test-with-low-deps branch fromdf5c849 to323168eCompare

December 12, 2017 10:36

Copy link

ContributorAuthor

Simperfit commentedDec 12, 2017

We can remove the deprecation tag ;)

nicolas-grekas removed the Deprecation label

Dec 12, 2017

nicolas-grekas reviewed

Dec 12, 2017

View reviewed changes

src/Symfony/Bridge/Twig/Tests/Extension/WorkflowExtensionTest.php Outdated

		$registry =newRegistry();
		$registry->add($workflow,newInstanceOfSupportStrategy(\stdClass::class));

		$registry->{method_exists($registry,$_ ='addWorkflow') ?$_ :'add'}($workflow,newInstanceOfSupportStrategy(\stdClass::class));

Copy link

Member

nicolas-grekasDec 12, 2017

Choose a reason for hiding this comment

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

$addWorkflow = method_exists($registry, 'addWorkflow') ? 'addWorkflow' : 'add';
$registry->$addWorkflow($workflow, new InstanceOfSupportStrategy(\stdClass::class));

Simperfit force-pushed thebugfix/fix-workflow-test-with-low-deps branch 2 times, most recently from1cd637b to1586984Compare

December 12, 2017 10:51

chalasr reviewed

Dec 12, 2017

View reviewed changes

src/Symfony/Bridge/Twig/Tests/Extension/WorkflowExtensionTest.php Outdated

		$addWorkflow =method_exists($registry,'addWorkflow') ?'addWorkflow' :'add';
		$supportStrategy =class_exists('Symfony\Component\Workflow\SupportStrategy\InstanceOfSupportStrategy')
		?new \Symfony\Component\Workflow\SupportStrategy\InstanceOfSupportStrategy(\stdClass::class)
		:new \Symfony\Component\Workflow\SupportStrategy\ClassInstanceSupportStrategy(\stdClass::class);

Copy link

Member

chalasrDec 12, 2017

Choose a reason for hiding this comment

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

please keep using use statements :)

[TwigBridge] Fix workflow test with deps=low

e5c6b92

Simperfit force-pushed thebugfix/fix-workflow-test-with-low-deps branch from1586984 toe5c6b92Compare

December 12, 2017 10:54

Copy link

ContributorAuthor

Simperfit commentedDec 12, 2017

🍾 it's green !

Copy link

Member

nicolas-grekas commentedDec 12, 2017

Thank you@Simperfit.

nicolas-grekas merged commite5c6b92 intosymfony:master

Dec 12, 2017

nicolas-grekas added a commit that referenced this pull request

Dec 12, 2017

minor#25458[TwigBridge] Fix workflow test with deps=low (Simperfit)

6f3de94

This PR was merged into the 4.1-dev branch.Discussion----------[TwigBridge] Fix workflow test with deps=low| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks?    | no| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->| Tests pass?   | yes| Fixed tickets | none| License       | MIT| Doc PR        | noneIn the original PR the tests were not failling but we should not use the new feature with deps=low until it fetch the branch with the new feature. So this fix the test for the WorkflowExtension in TwigBridge.Commits-------e5c6b92 [TwigBridge] Fix workflow test with deps=low

Simperfit deleted the bugfix/fix-workflow-test-with-low-deps branch

December 12, 2017 11:08