Improvements #4

michalbundyra · 2018-03-22T09:27:53Z

more generic tests in delegators to use all types of factories
removed duplicated test as these are covered by more generic tests using all factory types
refactor expected exception assertion
added one more missing delegator test

The same functionality is tested in testDelegatorFactoriesTriggerForFactoryBackedServicesUsingAnyFactoryType with data "invokable-class-name"

Use dataProvider delegatorService for all valid services which can be delegated

michalbundyra · 2018-03-22T15:57:13Z

With all these changes Aura.Di and Pimple integration PRs works:

weierophinney

This looks mostly good.. with the major exception of the DelegatorTestTrait. That class appears to be almost entirely rewritten, without any commentary on why.

I'm not sure if this was based on an earlier version of #1, or if there are reasons for the changes, but I can't merge this as-is currently because I don't understand why the changes were made.

weierophinney · 2018-04-04T21:34:05Z

src/AliasTestTrait.php

@@ -13,7 +13,7 @@

 trait AliasTestTrait
 {
-    public function alias() : Generator
+    final public function alias() : Generator


Just noting for the record, in case anybody wonders about the final visibility: this is because we do not want implementors overriding these tests and testing different behavior.

weierophinney · 2018-04-04T21:43:46Z

src/DelegatorTestTrait.php

@@ -63,31 +63,7 @@ public function testDelegatorsDoNotOperateOnServices() : void
        self::assertSame($myService, $instance);
    }

-    public function testDelegatorsOperateOnFactoryBackedServices() : void


Why is this test case removed?

Covered by testDelegatorFactoriesTriggerForFactoryBackedServicesUsingAnyFactoryType with data provider factoriesForDelegators (so more generic, for all types of factories).

weierophinney · 2018-04-04T21:44:43Z

src/DelegatorTestTrait.php

@@ -143,67 +119,64 @@ public function testDelegatorsNamedForAliasDoNotApplyToInvokableServiceResolvedV
        self::assertSame($instance, $container->get(TestAsset\Service::class));
    }

-    public function testDelegatorsDoNotApplyToAliasResolvingToServiceEntry() : void
+    final public function testDelegatorsNamedForAliasDoNotApplyToInvokableServiceWithAlias() : void


Why are these two tests swapped? It makes understanding the diff far harder (I had to look back and forth around a half-dozen times to realize that was what happened...)

Diff is messed up. Test testDelegatorsNamedForAliasDoNotApplyToInvokableServiceWithAlias was added here, as it is similar to the previous so it was logic place to add it.

Test testDelegatorsDoNotApplyToAliasResolvingToServiceEntry has not been modified at all, just added final.

testDelegatorsDoNotApplyToAliasResolvingToServiceEntry was moved; it now appears after the new test method.

It was not moved, another one was added before it, if you check "split view" maybe it will be clearer.

weierophinney · 2018-04-04T21:47:46Z

src/DelegatorTestTrait.php

@@ -231,39 +204,7 @@ public function testDelegatorsDoNotTriggerForAliasTargetingInvokableService() :
        self::assertSame($instance, $container->get(TestAsset\Service::class));
    }

-    public function testDelegatorsTriggerForFactoryServiceResolvedByAlias() : void


I found where this was moved, and discovered you added a data provider, which is excellent... but why was it moved? (Moving things around makes code review sooooo hard!)

It was moved to keep logical order, I've used dataProvders to provide more generic tests for all factory types.

I agree that moving things around makes code review harder. I think it was needed here to keep it clear in code later on.

weierophinney · 2018-04-04T21:48:30Z

src/DelegatorTestTrait.php

@@ -342,7 +280,32 @@ public function testDelegatorsReceiveCallbackResolvingToReturnValueOfPrevious(
        self::assertSame($instance, $container->get($serviceNameToTest));
    }

-    public function testMultipleAliasesForADelegatedInvokableServiceReceiveSameInstance()


Again: why was this test moved?

It was not moved, just diff is showing it wrong. New test was added before that one, and here only final keyword was added.

weierophinney · 2018-04-04T21:51:00Z

src/DelegatorTestTrait.php

    }
-
-    // @codingStandardsIgnoreStart
-    public function testWhenInvokableWithDelegatorsResolvesToInvalidFactoryClassAnExceptionIsRaisedWhenCallbackIsInvoked()


Why was this test removed?

Covered by more generic test testWithDelegatorsResolvesToInvalidClassAnExceptionIsRaisedWhenCallbackIsInvoked with data provider invalidService.

weierophinney · 2018-04-04T21:51:28Z

src/DelegatorTestTrait.php

-    }
-
-    // @codingStandardsIgnoreStart
-    public function testWhenServiceWithDelegatorsResolvesToNonExistentFactoryClassNoExceptionIsRaisedIfCallbackNeverInvoked()


Why was this test removed?

Covered by testWithDelegatorsResolvesToInvalidClassNoExceptionIsRaisedIfCallbackNeverInvoked with data provider invalidService.

weierophinney · 2018-04-04T21:51:49Z

src/DelegatorTestTrait.php

-    }
-
-    // @codingStandardsIgnoreStart
-    public function testWhenServiceWithDelegatorsResolvesToInvalidFactoryClassAnExceptionIsRaisedIfCallbackNeverInvoked()


Why was this test removed?

The test name was wrong, it was testing that "no exception is raised ...".

Covered by testWithDelegatorsResolvesToInvalidClassNoExceptionIsRaisedIfCallbackNeverInvoked with data provider invalidService

weierophinney · 2018-04-04T21:52:16Z

src/DelegatorTestTrait.php

        self::assertInstanceOf(TestAsset\Delegator::class, $instance);
    }
-
-    // @codingStandardsIgnoreStart
-    public function testWhenServiceWithDelegatorsResolvesToNonExistentFactoryClassAnExceptionIsRaisedWhenCallbackIsInvoked()


Why was this test removed?

Covered by testWithDelegatorsResolvesToInvalidClassAnExceptionIsRaisedWhenCallbackIsInvoked with data provider invalidService.

weierophinney · 2018-04-04T21:52:35Z

src/DelegatorTestTrait.php

-        $container->get('service');
-    }
-
-    public function testWhenServiceWithDelegatorsResolvesToInvalidFactoryClassAnExceptionIsRaisedWhenCallbackIsInvoked()


Why was this test removed?

Covered by testWithDelegatorsResolvesToInvalidClassAnExceptionIsRaisedWhenCallbackIsInvoked with data provider invalidService

michalbundyra · 2018-04-04T22:11:31Z

@weierophinney I think the diff is messed up somehow, it's better to have a look on separate commits. Basically I've added data providers to have more cases and removed duplicated tests (as data providers covers all cases). I can write more explanations tomorrow, but I think when you have a look commit by commit it should be clearer.

michalbundyra · 2018-04-05T06:55:13Z

src/DelegatorTestTrait.php

    }

-    public function testDelegatorsDoNotTriggerForAliasTargetingFactoryBasedService() : void


This one is removed, because it's covered by testDelegatorsDoNotTriggerForAliasTargetingFactoryBasedServiceUsingAnyFactoryType.

michalbundyra · 2018-04-05T06:59:22Z

src/DelegatorTestTrait.php

@@ -281,14 +222,6 @@ public function delegatorService() : Generator
            TestAsset\Service::class,
        ];

-        yield 'factory-service' => [


Check foreach loop below - we are generating there entities for all factory types, the same about alias-of-factory-service.

michalbundyra · 2018-04-05T07:21:24Z

src/DelegatorTestTrait.php

-        try {
-            $container->get(TestAsset\NonExistent::class);
-        } catch (Throwable $e) {
-            if (! $e instanceof Error && ! $e instanceof TypeError && ! $e instanceof ContainerExceptionInterface) {


Here we allow Error, TypeError and ContainerExceptionInterface.
Please note that TypeError extends Error so it's no point to use both here - Error is enough. And this is why in testWithDelegatorsResolvesToInvalidClassAnExceptionIsRaisedWhenCallbackIsInvoked we use only Error and ContainerExceptionInterface.

Improvements

michalbundyra added 4 commits March 22, 2018 08:20

Removed duplicated test

5ef6cd2

The same functionality is tested in testDelegatorFactoriesTriggerForFactoryBackedServicesUsingAnyFactoryType with data "invokable-class-name"

Updated tests to be more generic - using any factory type

9c3bccf

Added more delegator tests

ee8ba14

Added helper to expect one of the defined exception to be thrown

6324a5c

michalbundyra requested a review from weierophinney March 22, 2018 09:28

michalbundyra added 3 commits March 22, 2018 10:22

Updated test to be more generic with invalid delegators

13fa5d8

Use dataProvider delegatorService for all valid services which can be delegated

Added dataProvider for invalid service in delegators

732e61b

CS fix

8e64831

michalbundyra force-pushed the improvements branch from 4a703db to 8e64831 Compare March 22, 2018 10:47

michalbundyra added 4 commits March 22, 2018 10:57

Added two more cases with non-invokable factories

cd4554d

Updated methods signature - all final, tests return void

33a5150

Added test for non existent invokable class

5268853

Added test for empty delegator list

aea2454

This was referenced Mar 22, 2018

Expressive compatibility jsoumelidis/zend-sf-di-config#2

Closed

Expressive compatibility northwoods/container#11

Closed

weierophinney closed this Apr 4, 2018

weierophinney changed the base branch from feature/review-tests to master April 4, 2018 21:32

weierophinney reopened this Apr 4, 2018

weierophinney reviewed Apr 4, 2018

View reviewed changes

michalbundyra commented Apr 5, 2018

View reviewed changes

weierophinney merged commit aea2454 into master Apr 5, 2018

weierophinney added a commit that referenced this pull request Apr 5, 2018

Merge pull request #4 from weierophinney/improvements

9125da0

Improvements

weierophinney deleted the improvements branch April 5, 2018 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements #4

Improvements #4

michalbundyra commented Mar 22, 2018

michalbundyra commented Mar 22, 2018

weierophinney left a comment

weierophinney Apr 4, 2018

weierophinney Apr 4, 2018

michalbundyra Apr 5, 2018

weierophinney Apr 4, 2018

michalbundyra Apr 5, 2018

weierophinney Apr 5, 2018

michalbundyra Apr 5, 2018

weierophinney Apr 4, 2018

michalbundyra Apr 5, 2018

weierophinney Apr 4, 2018

michalbundyra Apr 5, 2018 •

edited

Loading

weierophinney Apr 4, 2018

michalbundyra Apr 5, 2018

weierophinney Apr 4, 2018

michalbundyra Apr 5, 2018

weierophinney Apr 4, 2018

michalbundyra Apr 5, 2018

weierophinney Apr 4, 2018

michalbundyra Apr 5, 2018

weierophinney Apr 4, 2018

michalbundyra Apr 5, 2018

michalbundyra commented Apr 4, 2018

michalbundyra Apr 5, 2018

michalbundyra Apr 5, 2018

michalbundyra Apr 5, 2018

		}

		public function testDelegatorsDoNotTriggerForAliasTargetingFactoryBasedService() : void

Improvements #4

Improvements #4

Conversation

michalbundyra commented Mar 22, 2018

michalbundyra commented Mar 22, 2018

weierophinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalbundyra Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalbundyra commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalbundyra Apr 5, 2018 •

edited

Loading