Fix StaticValidator::execute tests and behavior #26

weierophinney · 2015-07-21T18:27:13Z

This is an attempt to address #16.

Previously, the tests were incorrectly passing a non-associative array of options to execute(), which implied that they would be passed as constructor arguments, which was not the case. In this particular case, there was a false positive, as the value being validated was valid for the default values present in the instance.

This patch removes that assertion, and splits the test into two, one for validating properly formed calls to the method with options as an associative array, and one for calling it with non-associative options. In the second, the test asserts that an InvalidArgumentException is expected.

The patch is a potential BC break. However, passing a non-associative array of arguments had unexpected behavior previously, and relying on that behavior would result in application bugs. The change to raise an exception ensures consistent behavior.

Previously, the tests were incorrectly passing a non-associative array of options to `execute()`, which implied that they would be passed as constructor arguments, which was not the case. In this particular case, there was a false positive, as the value being validated was valid for the default values present in the instance. This patch removes that assertion, and splits the test into two, one for validating properly formed calls to the method with options as an associative array, and one for calling it with non-associative options. In the second, the test asserts that an `InvalidArgumentException` is expected.

The ValidatorChainTest suite duplicated StaticValidator tests - particularly the faulty ones this patch is attempting to fix.

Maks3w · 2015-07-21T20:01:14Z

src/StaticValidator.php

@@ -52,6 +52,12 @@ public static function getPluginManager()
     */
    public static function execute($value, $classBaseName, array $args = [])
    {
+        if ($args && array_values($args) === $args) {


Should this check to be done in the PluginManager?

Forgive this

Maks3w · 2015-07-21T20:18:02Z

I see three cases here:

Constructor uses func_get_args
$args is used for constructor arguments so could be a bidimensional array passing the options as first argument
`[ [ options ] ]
$args is the array options accidentaly used as constructor arguments list

Because argument name is $args which means arguments list I think the current implementation is the correct. The intention of provide a constructor argument list.

Provide a constructor argument list allow to instantiate constructors with different constructor signatures and not only the ones based in AbstractValidator.

My opinion is not merge and won't fix.

Maks3w · 2015-07-21T20:22:56Z

I've forwarded my comment to the issue for to have a single thread.

weierophinney · 2015-07-21T20:23:19Z

Constructor uses func_get_args

This one is a moot point; the plugin manager always passes arguments (options) as an array, never using variadics.

$args is used for constructor arguments so could be a bidimensional array passing the options as first argument [ [ options ] ]

This does not occur currently.

$args is the array options accidentaly used as constructor arguments list

$args is passed as the $options argument to the plugin manager instance, and $options, if present, is always passed as the sole argument to the plugin constructor. The specified use case when we designed this was precisely for passing an associative array (or array-like object) of options to the plugin, to allow setting its initial state.

Which completely invalidates this assertion:

Because argument name is $args which means arguments list I think the current implementation is the correct. The intention of provide a constructor argument list.

That was never the intention. If it were, I would have coded the AbstractPluginManager differently. This is simply a case of (a) misnamed argument, and (b) incorrect initial tests. Somebody essentially discovered that we were not covering an edge case properly.

With that knowledge, please re-review.

- Adds an `@throws` annotation to `execute()` - Renames `$args` to `$options` to better indicate that it is passed as the `$options` argument to `ValidatorPluginManager::get()`.

Maks3w · 2015-07-21T21:08:22Z

src/StaticValidator.php

@@ -47,14 +47,22 @@ public static function getPluginManager()
    /**
     * @param  mixed    $value
     * @param  string   $classBaseName
-     * @param  array    $args          OPTIONAL
+     * @param  array    $optons OPTIONAL options to pass to the validator constructor;


typo s/optons/options

Include "as first argument". "associative array of options for to inject in the first argument of the validator constructor"

Maks3w · 2015-07-22T07:21:49Z

test/StaticValidatorTest.php

+     */
+    public function testExecuteValidWithParameters($value, $validator, $options, $assertion)
+    {
+        $this->$assertion(StaticValidator::execute($value, $validator, $options));


Lately I prefer asser the type first and then the value

asserInternalType(boolean, return)
assertEquals(expected, return)

The first ensure the type is a documented return type and also force expected to be the same type
The second one is the classic discrete comparison of values

This format has 2 goodnes:

Keep clen the data provider from phpunit internals (method names)

Keep a consistent test format with a more clear error messages

In this case, it can be simplified even further: assertSame($expected, $return), which is the approach I'm about to push.

The problem with assersame is anyone can put a string in the provider.

Split in two make more clear 1:1 relations with method signature and avoid put false positive values in provider.

This is motivated in cases where the provider is implemented in child classes while the test method is not

The contract for validators is to return boolean. Adding the assertion on
type is superfluous.

On Wed, Jul 22, 2015 at 8:01 AM, Maks3w notifications@github.com wrote:

In test/StaticValidatorTest.php
#26 (comment)
:

$this->assertTrue(StaticValidator::execute(5, 'Between', [1, 10]));

$this->assertTrue(StaticValidator::execute(5, 'Between', ['min' => 1, 'max' => 10]));

return [

'valid-positive-range' => [5, 'Between', ['min' => 1, 'max' => 10], 'assertTrue'],

'valid-negative-range' => [-5, 'Between', ['min' => -10, 'max' => -1], 'assertTrue'],

'invalid-positive-range' => [-5, 'Between', ['min' => 1, 'max' => 10], 'assertFalse'],

'invalid-negative-range' => [5, 'Between', ['min' => -10, 'max' => -1], 'assertFalse'],

];

}

/**

\* @dataProvider parameterizedData

*/

public function testExecuteValidWithParameters($value, $validator, $options, $assertion)

{

$this->$assertion(StaticValidator::execute($value, $validator, $options));

The problem with assersame is anyone can put a string in the provider.

Split in two make more clear 1:1 relations with method signature and avoid
put false positive values in provider.

This is motivated in cases where the provider is implemented in child
classes while the test method is not

—
Reply to this email directly or view it on GitHub
https://github.com/zendframework/zend-validator/pull/26/files#r35209413.

Matthew Weier O'Phinney
matthew@weierophinney.net
https://mwop.net/

- Pass expected value from provider, and use `assertSame()`.

Forward port #26

weierophinney added bug BC Break awaiting maintainer response labels Jul 21, 2015

weierophinney added this to the 2.5.3 milestone Jul 21, 2015

Remove duplicate test

fac4c45

The ValidatorChainTest suite duplicated StaticValidator tests - particularly the faulty ones this patch is attempting to fix.

Maks3w reviewed Jul 21, 2015
View reviewed changes

Maks3w mentioned this pull request Jul 21, 2015

StaticValidator test is incorrect (false positive) #16

Closed

Incorporate feedback

6ca5e0a

- Adds an `@throws` annotation to `execute()` - Renames `$args` to `$options` to better indicate that it is passed as the `$options` argument to `ValidatorPluginManager::get()`.

Maks3w reviewed Jul 21, 2015
View reviewed changes

Incorporate docblock feedback

79e8c96

Maks3w reviewed Jul 22, 2015
View reviewed changes

Use standard assertion mechanics

763ed81

- Pass expected value from provider, and use `assertSame()`.

weierophinney removed the awaiting maintainer response label Jul 22, 2015

weierophinney added a commit that referenced this pull request Jul 22, 2015

Created CHANGELOG for #26

3089115

weierophinney added a commit that referenced this pull request Jul 22, 2015

Merge branch 'hotfix/16' into develop

03348a2

Forward port #26

weierophinney closed this in 912de1f Jul 22, 2015

weierophinney self-assigned this Jul 22, 2015

weierophinney deleted the hotfix/16 branch July 23, 2015 13:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix StaticValidator::execute tests and behavior #26

Fix StaticValidator::execute tests and behavior #26

weierophinney commented Jul 21, 2015

Maks3w Jul 21, 2015

Maks3w Jul 21, 2015

Maks3w commented Jul 21, 2015

Maks3w commented Jul 21, 2015

weierophinney commented Jul 21, 2015

Maks3w Jul 21, 2015

Maks3w Jul 22, 2015

weierophinney Jul 22, 2015

Maks3w Jul 22, 2015

weierophinney Jul 22, 2015

Fix StaticValidator::execute tests and behavior #26

Fix StaticValidator::execute tests and behavior #26

Conversation

weierophinney commented Jul 21, 2015

Maks3w Jul 21, 2015

Choose a reason for hiding this comment

Maks3w Jul 21, 2015

Choose a reason for hiding this comment

Maks3w commented Jul 21, 2015

Maks3w commented Jul 21, 2015

weierophinney commented Jul 21, 2015

Maks3w Jul 21, 2015

Choose a reason for hiding this comment

Maks3w Jul 22, 2015

Choose a reason for hiding this comment

weierophinney Jul 22, 2015

Choose a reason for hiding this comment

Maks3w Jul 22, 2015

Choose a reason for hiding this comment

weierophinney Jul 22, 2015

Choose a reason for hiding this comment