Remove zend json from the test suite #10320

dmanners · 2017-07-25T13:34:06Z

Description

Since Zend_Json is at EOL we should replace the usage of \Magento\Framework\Serialize\Serializer\Json when required. In this PR I have removed the usage of Zend_Json from the test suite.

Fixed Issues (if relevant)

Upgrade ZF components. Zend_Json #9236: Upgrade ZF components. Zend_Json

Manual testing scenarios

Running the test suite causes no failures

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

- replace with direct calls to json_decode and json_encode as these are test cases

…ct json_ methods

- Mark AbstractController to ignore CouplingBetweenObjects - Set the same json_decode default that Zend_Json uses

dmanners · 2017-07-25T13:35:21Z

dev/tests/integration/framework/Magento/TestFramework/TestCase/AbstractController.php

@@ -268,14 +269,21 @@ protected function getCookieMessages($messageType = null)
    {
        /** @var $cookieManager CookieManagerInterface */
        $cookieManager = $this->_objectManager->get(CookieManagerInterface::class);
+
+        /** @var $jsonSerializer \Magento\Framework\Serialize\Serializer\Json */
+        $jsonSerializer = $this->_objectManager->get(\Magento\Framework\Serialize\Serializer\Json::class);


Now I was not sure if I should use the class \Magento\Framework\Serialize\Serializer\Json here or just json_* but I thought since this was using the cookie interface it should also use the serializer. Happy to make a change if it is thought best to do something else.

As we suppress exception anyway here I would simplify implementation and got rid of try/catch at all. But as it is just testing code it does not really matter.

ishakhsuvarov · 2017-07-25T13:56:30Z

dev/tests/integration/framework/Magento/TestFramework/TestCase/AbstractController.php

@@ -15,6 +15,7 @@

 /**
 * @SuppressWarnings(PHPMD.NumberOfChildren)
+ * @SuppressWarnings(PHPMD.CouplingBetweenObjects)


Adding SuppressWarnings, specially for Coupling, is generally considered a bad practice.
Did you add this as a result of tests execution? As it looks like that you have added same amount of dependencies as removed.
Usually in such cases refactoring has to be done to decouple objects

@ishakhsuvarov I added this as the static test limit got hit because of adding in https://github.com/magento/magento2/pull/10320/files#diff-22fb93f06b955235735842cbdd6c2ab3R274 which I am still not sure about since this is inside the test framework itself.

But at the same time you have decoupled this class from \Zend_Json and \Zend_Json_Exception. So to me it looks like that same coupling value should be in place after your changes.
Am I missing something?

I am not sure myself. https://travis-ci.org/dmanners/magento2/jobs/257261073 that was the failure. I wonder if PHPMD runs from a diff of classes changed?

Yes, it is executed from a diff, looks like it was already broken before.
I will look into the possible ways to reduce coupling for this class.

Thanks, let me know if I can help out here.

I've added some changes, which reduced coupling, in a way, which seems reasonable to me. Going to proceed with the merge now. Please let me know if it's ok from your perspective.

Yeah those changes seem good to me still not 100% sure why that change fixes the static check, but I can live with not knowing that currently 👍

- Attempt to reduce coupling between objects

- Fixed unit test for the integration tests framework

dmanners · 2017-07-25T16:03:00Z

.../integration/framework/tests/unit/testsuite/Magento/Test/TestCase/ControllerAbstractTest.php

+        $this->serializerMock = $this->getMockBuilder(\Magento\Framework\Serialize\Serializer\Json::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $this->serializerMock->expects($this->any())->method('unserialize')->willReturnCallback(


Oh I love this approach for mocking methods

I tend to think that using real dependency in this case would be a better idea, however we usually stick to mocking all the stuff in the unit tests.

yeah when it comes to mocking or not mocking I am not so sure what is "best" but the callback here is a nice idea specifically for this mock.

Such "mock" is practically equivalent to a real class 😉 I prefer tiny tests with small fixtures having mocks in form of returning plain values. Copy-pasting such callbacks to dozens of tests somewhat increases maintenance efforts.

orlangur · 2017-07-25T17:10:28Z

dev/tests/integration/framework/Magento/TestFramework/TestCase/AbstractController.php

@@ -268,14 +269,21 @@ protected function getCookieMessages($messageType = null)
    {
        /** @var $cookieManager CookieManagerInterface */
        $cookieManager = $this->_objectManager->get(CookieManagerInterface::class);
+
+        /** @var $jsonSerializer \Magento\Framework\Serialize\Serializer\Json */
+        $jsonSerializer = $this->_objectManager->get(\Magento\Framework\Serialize\Serializer\Json::class);


As we suppress exception anyway here I would simplify implementation and got rid of try/catch at all. But as it is just testing code it does not really matter.

orlangur · 2017-07-25T17:14:46Z

.../integration/framework/tests/unit/testsuite/Magento/Test/TestCase/ControllerAbstractTest.php

+        $this->serializerMock = $this->getMockBuilder(\Magento\Framework\Serialize\Serializer\Json::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $this->serializerMock->expects($this->any())->method('unserialize')->willReturnCallback(


Such "mock" is practically equivalent to a real class 😉 I prefer tiny tests with small fixtures having mocks in form of returning plain values. Copy-pasting such callbacks to dozens of tests somewhat increases maintenance efforts.

dmanners added 3 commits July 25, 2017 07:33

Remove the Zend_Json usage in the integration test suite and framework

474effb

- replace with direct calls to json_decode and json_encode as these are test cases

Update the abstract controller to use the Serializer rather than dire…

db624ff

…ct json_ methods

Fix-up some probelms with test suite

1ad0251

- Mark AbstractController to ignore CouplingBetweenObjects - Set the same json_decode default that Zend_Json uses

dmanners commented Jul 25, 2017

View reviewed changes

ishakhsuvarov self-assigned this Jul 25, 2017

ishakhsuvarov added this to the July 2017 milestone Jul 25, 2017

ishakhsuvarov added the develop label Jul 25, 2017

ishakhsuvarov reviewed Jul 25, 2017

View reviewed changes

ishakhsuvarov added 2 commits July 25, 2017 17:50

magento#10320: Remove zend json from the test suite

2867a11

- Attempt to reduce coupling between objects

magento#10320: Remove zend json from the test suite

8cd3d3a

- Fixed unit test for the integration tests framework

dmanners commented Jul 25, 2017

View reviewed changes

magento-team added Progress: accept develop labels Jul 25, 2017

orlangur approved these changes Jul 25, 2017

View reviewed changes

magento-team merged commit 8cd3d3a into magento:develop Jul 25, 2017

magento-team pushed a commit that referenced this pull request Jul 25, 2017

MAGETWO-71007: Remove zend json from the test suite #10320

7371346

dmanners deleted the remove-zend-json-from-test-suite branch July 26, 2017 12:01

magento-team pushed a commit that referenced this pull request Oct 3, 2017

MAGETWO-80179: Remove zend json from the test suite #10320

422a16e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove zend json from the test suite #10320

Remove zend json from the test suite #10320

dmanners commented Jul 25, 2017 •

edited by ishakhsuvarov

Loading

dmanners Jul 25, 2017

orlangur Jul 25, 2017

ishakhsuvarov Jul 25, 2017

dmanners Jul 25, 2017

ishakhsuvarov Jul 25, 2017

dmanners Jul 25, 2017

ishakhsuvarov Jul 25, 2017

dmanners Jul 25, 2017

ishakhsuvarov Jul 25, 2017

dmanners Jul 25, 2017

dmanners Jul 25, 2017

ishakhsuvarov Jul 25, 2017

dmanners Jul 25, 2017

orlangur Jul 25, 2017

orlangur Jul 25, 2017

orlangur Jul 25, 2017

Remove zend json from the test suite #10320

Remove zend json from the test suite #10320

Conversation

dmanners commented Jul 25, 2017 • edited by ishakhsuvarov Loading

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

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

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

dmanners commented Jul 25, 2017 •

edited by ishakhsuvarov

Loading