Skip to content
This repository was archived by the owner on Apr 29, 2019. It is now read-only.

Removed create_function() function usage #32

Merged
merged 1 commit into from
Feb 8, 2018
Merged

Removed create_function() function usage #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions dev/tests/static/testsuite/Magento/Test/Legacy/_files/obsolete_methods.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2528,4 +2528,5 @@
['_isAttributeValueEmpty', 'Magento\Catalog\Model\ResourceModel\AbstractResource'],
['var_dump', ''],
['each', ''],
['create_function', ''],
];
99 changes: 60 additions & 39 deletions lib/internal/Magento/Framework/Validator/Test/Unit/Constraint/Option/CallbackTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

namespace Magento\Framework\Validator\Test\Unit\Constraint\Option;

/**
* Test case for \Magento\Framework\Validator\Constraint\Option\Callback
*/
use Magento\Framework\Validator\Constraint\Option\Callback;
use Magento\Framework\Validator\Test\Unit\Test\Callback as TestCallback;

class CallbackTest extends \PHPUnit\Framework\TestCase
{
/**
Expand All @@ -28,41 +27,46 @@ class CallbackTest extends \PHPUnit\Framework\TestCase
*/
public function testGetValue($callback, $expectedResult, $arguments = null, $createInstance = false)
{
$option = new \Magento\Framework\Validator\Constraint\Option\Callback($callback, $arguments, $createInstance);
$this->assertEquals($expectedResult, $option->getValue());
$option = new Callback($callback, $arguments, $createInstance);
self::assertEquals($expectedResult, $option->getValue());
}

/**
* Data provider for testGetValue
*/
public function getConfigDataProvider()
{
$functionName = create_function('', 'return "Value from function";');
$closure = function () {
return 'Value from closure';
};

$mock = $this->getMockBuilder('Foo')->setMethods(['getValue'])->getMock();
$mock->expects(
$this->once()
)->method(
'getValue'
)->with(
'arg1',
'arg2'
)->will(
$this->returnValue('Value from mock')
);
$mock = $this->getMockBuilder('Foo')
->setMethods(['getValue'])
->getMock();
$mock->method('getValue')
Copy link
Contributor

@ihor-sviziev ihor-sviziev Feb 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like section ->expects($this->once()) is missing here

Copy link
Contributor Author

@YevSent YevSent Feb 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It doesn't matter how many times getValue will be called, that's why expects is not needed.

Copy link
Contributor

@ihor-sviziev ihor-sviziev Feb 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

In this case would be better to use ->expects($this->any()) to make it clear

Copy link
Contributor Author

@YevSent YevSent Feb 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

For this test case, in general, it doesn't matter how many times the method will be called, so any() is redundant. The expects() may sense to use when we need to know if the method is called or not (corner cases, like business logic conditionals) or how many times it was called - for performance reasons.
I don't see, how any() makes test a cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. Good point. Let's leave it as is

->with('arg1', 'arg2')
->willReturn('Value from mock');

return [
[$functionName, 'Value from function'],
[$closure, 'Value from closure'],
[[$this, 'getTestValue'], self::TEST_VALUE],
[[__CLASS__, 'getTestValueStatically'], self::TEST_VALUE],
[[$mock, 'getValue'], 'Value from mock', ['arg1', 'arg2']],
[
[\Magento\Framework\Validator\Test\Unit\Test\Callback::class, 'getId'],
\Magento\Framework\Validator\Test\Unit\Test\Callback::ID,
$closure,
'Value from closure'
],
[
[$this, 'getTestValue'],
self::TEST_VALUE
],
[
[__CLASS__, 'getTestValueStatically'],
self::TEST_VALUE
],
[
[$mock, 'getValue'],
'Value from mock', ['arg1', 'arg2']
],
[
[TestCallback::class, 'getId'],
TestCallback::ID,
null,
true
]
Expand Down Expand Up @@ -90,25 +94,29 @@ public function getTestValue()
*
* @dataProvider setArgumentsDataProvider
*
* @param mixed $value
* @param mixed $expectedValue
* @param string|array $value
* @param string|array $expectedValue
*/
public function testSetArguments($value, $expectedValue)
{
$option = new \Magento\Framework\Validator\Constraint\Option\Callback(
function () {
}
);
$option = new Callback(function () {
});
$option->setArguments($value);
$this->assertAttributeEquals($expectedValue, '_arguments', $option);
self::assertAttributeEquals($expectedValue, '_arguments', $option);
}

/**
* Data provider for testGetValue
*/
public function setArgumentsDataProvider()
{
return [['baz', ['baz']], [['foo', 'bar'], ['foo', 'bar']]];
return [
['baz', ['baz']],
[
['foo', 'bar'],
['foo', 'bar']
]
];
}

/**
Expand All @@ -119,11 +127,12 @@ public function setArgumentsDataProvider()
* @param mixed $callback
* @param string $expectedMessage
* @param bool $createInstance
* @expectedException \InvalidArgumentException
*/
public function testGetValueException($callback, $expectedMessage, $createInstance = false)
{
$option = new \Magento\Framework\Validator\Constraint\Option\Callback($callback, null, $createInstance);
$this->expectException('InvalidArgumentException', $expectedMessage);
$option = new Callback($callback, null, $createInstance);
self::expectExceptionMessage($expectedMessage);
$option->getValue();
}

Expand All @@ -139,10 +148,22 @@ public function getValueExceptionDataProvider()
['Not_Existing_Callback_Class', 'someMethod'],
'Class "Not_Existing_Callback_Class" was not found',
],
[[$this, 'notExistingMethod'], 'Callback does not callable'],
[['object' => $this, 'method' => 'getTestValue'], 'Callback does not callable'],
['unknown_function', 'Callback does not callable'],
[new \stdClass(), 'Callback does not callable'],
[
[$this, 'notExistingMethod'],
'Callback does not callable'
],
[
['object' => $this, 'method' => 'getTestValue'],
'Callback does not callable'
],
[
'unknown_function',
'Callback does not callable'
],
[
new \stdClass(),
'Callback does not callable'
],
[
[$this, 'getTestValue'],
'Callable expected to be an array with class name as first element',
Expand Down