Improves SapiStreamEmitter tests, preventing unnecessary string casts #226

fcabralpacheco · 2017-01-22T16:48:14Z

@Ocramius

I tried to improve some points in the SapiStreamEmitter tests:

Reduced code repetition by creating a new method for the Stream Prophecy common code.
Added a method for expanding the test data set. This facilitates the addition of new data set with one information changed (e.g.: the contents).
Improved checks of methods that can or cannot be called.
Removed the expectation of read counts because the buffer size is the maximum, the implementation can use a smaller one if you want.
Considering bug JsonResponse not working #225, which I was not able to reproduce, both in the test and in an Expressive application. Added new tests for each type of response, checking what was emitted, and whether the content type was not changed.

Just a few comments:

I didn't add out of range conditions to the tests (in testEmitRangeStreamResponse). I think that the application should handle this type of case, and it may be not possible to detect this type of condition in non seekable bodies.
Please keep prophecy prediction after reveal. The Prophecy don't calls promise if it signature don't match exact with prediction signature.
E.g.: $stream->seek(Argument::type('integer'), Argument::any()); vs $stream->seek($first)->should($seekPredictionClosure); and $ stream->seek($first, SEEK_SET)->should($seekPredictionClosure);
This occurs because signatures generate three different prophecies, one with a promise and two with a prediction, on execution only highest rated prophecy is executed. If you pass the "$first" value, only first prediction would be executed.

Linked to #224, #223

…est data sets.

…tTextResponse. Impovement in testEmitJsonResponse (check if content-type has changed). Minor CS fixes.

Ocramius · 2017-01-22T17:01:30Z

test/Response/SapiStreamEmitterTest.php

+    {
+        $stream = $this->prophesize('Psr\Http\Message\StreamInterface');
+
+        $stream->__toString()->will(function () use (& $contents, & $position, $size) {


Is $size supposed to not be passed in by reference?

The $size don't need by ref.

Ocramius · 2017-01-22T17:01:46Z

test/Response/SapiStreamEmitterTest.php

+            }
+        };
+
+        array_walk(array_shift($initialData), $closureExpand, [


Please use consistent alignment (PSR-2)

Ocramius · 2017-01-22T17:02:06Z

test/Response/SapiStreamEmitterTest.php

+
+        $resultData = [];
+
+        $closureExpand = function ($item, $key, $parameters) use (& $closureExpand) {


What does closureExpand mean here?

The method expands data like a tree. In the end of branch resides one data set.

Ocramius · 2017-01-22T17:02:45Z

test/Response/SapiStreamEmitterTest.php

+        return $resultData;
+    }
+
+    private function getStreamProphecy(& $contents, & $size, & $position, & $peakBufferLength)


s/get/build

The act that all this stuff is by-ref makes it extremely hard to read

Ok. The $size don't need by ref, the $contents too (I thought it would be necessary at first due to memory usage).

The $position and $peakBufferLength is necessary, I can return them in an array. Like this:
private function getStreamProphecy($contents, $size)
...
return [
$stream,
& $position,
& $peakBufferLength,
];

And I can rename getStreamProphecy to setUpStreamProphecy.

What do you think?

Removed unecessary by-ref and replaced $peakBufferLength by a callable.
I think it's now become more intuitive.

Ocramius · 2017-01-22T17:04:41Z

test/Response/SapiStreamEmitterTest.php

+        return $stream;
+    }
+
+    public function emitStreamResponseProvider()


Please document the return type o this data provider

@Ocramius
I'm not sure if after the changes this still necessary.
Also, I don't know exactly how to document elements of an array according to the ZF CS (as far as I know. There is no official way to do this in phpdoc).

Oh, it just needed a human-friendly description. The new syntax makes it much more approachable, so it's not needed anymore.

Ocramius · 2017-01-22T17:06:58Z

test/Response/SapiStreamEmitterTest.php

-        });
+    public function emitJsonResponseProvider()
+    {
+        return [[0.1                                                                         ],


Alignment broken here

Ocramius · 2017-01-22T17:07:08Z

test/Response/SapiStreamEmitterTest.php

-            ['bytes 0-2/*', 'Hello world', 'Hel'],
-            ['bytes 3-6/*', 'Hello world', 'lo w'],
-            ['items 0-0/1', 'Hello world', 'Hello world'],
+                ['bytes 0-2/*', 'Hello world', 'Hel'],


Alignment broken here

Okay. It was supposed to be just a reordering of this piece of the code. I messed things.

Ocramius · 2017-01-22T17:09:42Z

test/Response/SapiStreamEmitterTest.php

@@ -57,76 +61,322 @@ public function testDoesNotInjectContentLengthHeaderIfStreamSizeIsUnknown()
        }
    }

-    public function emitBodyProvider()
+    private function expandTestData()


While I get that you wanted to normalize the mocking done for the data providers, this method has become absolutely unreadable. I would suggest duplicating or the supported scenarios. Specifically based on:

number of passed in parameters

types of passed in parameters

I get it. I'll use duplication.
I thought the same when I finished writing this method.
I tried a version without array_walk (using for, next, current), it got worse.

Ocramius · 2017-01-22T17:10:52Z

test/Response/SapiStreamEmitterTest.php

-            return $data;
-        });
+        if ($readable) {
+            $stream->__toString()->shouldNotBeCalled();


Most probably the important assertion that we missed before :S

I agree with you.

Ocramius · 2017-01-22T17:11:12Z

test/Response/SapiStreamEmitterTest.php

+    /**
+     * @dataProvider emitRangeStreamResponseProvider
+     */
+    public function testEmitRangeStreamResponse($seekable, $readable, array $range, $contents, $maxBufferLength)


Please document the parameter types/formats

…and minor bug (last byte position in testEmitMemoryUsage).

Ocramius · 2017-01-23T04:13:09Z

test/Response/SapiStreamEmitterTest.php

+        return $stream;
+    }
+
+    public function emitStreamResponseProvider()


Oh, it just needed a human-friendly description. The new syntax makes it much more approachable, so it's not needed anymore.

…ast' Close #226

Ocramius · 2017-01-23T04:16:41Z

Backported into master via c2c5982

fcabralpacheco · 2017-01-23T04:33:41Z

@Ocramius

I'm sorry, just a little type correction/type hinting.

fcabralpacheco@346e468

Thanks.

Ocramius · 2017-01-23T04:34:42Z

@fcabralpacheco cherry-picking it, thanks!

Ocramius · 2017-01-23T04:37:58Z

@fcabralpacheco also, sorry for having to do all this because of a messed up CS fix by me: thanks for all the patience!

fcabralpacheco · 2017-01-23T04:45:19Z

I'm sorry again, just a little correction.

fcabralpacheco@555e9cd

fcabralpacheco@e406769

Thanks

Ocramius · 2017-01-23T04:45:37Z

Already applied fcabralpacheco@555e9cd

fcabralpacheco · 2017-01-23T04:46:06Z

And thanks for all the patience too.

fcabralpacheco added 5 commits January 18, 2017 00:56

Improve SapiStreamSemitter tests.

d17861d

Reduced code repetition on emit tests. Added new checks. Added more t…

e5a0956

…est data sets.

Added new testEmitJsonResponse test. Minor CS fixes.

737d9c8

Added new tests: testEmitEmptyResponse, testEmitHtmlResponse, testEmi…

e221486

…tTextResponse. Impovement in testEmitJsonResponse (check if content-type has changed). Minor CS fixes.

Added missing returns on SapiStreamEmitter.php.

0a315fd

Ocramius suggested changes Jan 22, 2017

View reviewed changes

Ocramius assigned fcabralpacheco Jan 22, 2017

Ocramius added the enhancement label Jan 22, 2017

Ocramius added this to the 1.3.10 milestone Jan 22, 2017

Ocramius added Awaiting author feedback bug labels Jan 22, 2017

Removed expandTestData. Improved documentation. Fixes CS (alignment) …

764502c

…and minor bug (last byte position in testEmitMemoryUsage).

Ocramius approved these changes Jan 23, 2017

View reviewed changes

Ocramius changed the title ~~Improves SapiStreamEmitter tests~~ Improves SapiStreamEmitter tests, preventing unnecessary string casts Jan 23, 2017

Ocramius unassigned fcabralpacheco Jan 23, 2017

Ocramius removed the Awaiting author feedback label Jan 23, 2017

Ocramius merged commit 2faeff4 into zendframework:develop Jan 23, 2017

Ocramius added a commit that referenced this pull request Jan 23, 2017

Merge branch 'fix/#226-improve-sapi-stream-emitter-tests-and-string-c…

c2c5982

…ast' Close #226

Ocramius self-assigned this Jan 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improves SapiStreamEmitter tests, preventing unnecessary string casts #226

Improves SapiStreamEmitter tests, preventing unnecessary string casts #226

fcabralpacheco commented Jan 22, 2017

Ocramius Jan 22, 2017

fcabralpacheco Jan 22, 2017

Ocramius Jan 22, 2017

Ocramius Jan 22, 2017

fcabralpacheco Jan 22, 2017

Ocramius Jan 22, 2017

Ocramius Jan 22, 2017

fcabralpacheco Jan 22, 2017

fcabralpacheco Jan 22, 2017

Ocramius Jan 22, 2017

fcabralpacheco Jan 22, 2017

Ocramius Jan 23, 2017

Ocramius Jan 22, 2017

Ocramius Jan 22, 2017

fcabralpacheco Jan 22, 2017

Ocramius Jan 22, 2017

fcabralpacheco Jan 22, 2017

Ocramius Jan 22, 2017

fcabralpacheco Jan 22, 2017

Ocramius Jan 22, 2017

Ocramius Jan 23, 2017

Ocramius commented Jan 23, 2017

fcabralpacheco commented Jan 23, 2017

Ocramius commented Jan 23, 2017

Ocramius commented Jan 23, 2017

fcabralpacheco commented Jan 23, 2017

Ocramius commented Jan 23, 2017

fcabralpacheco commented Jan 23, 2017


		$resultData = [];

		$closureExpand = function ($item, $key, $parameters) use (& $closureExpand) {

Improves SapiStreamEmitter tests, preventing unnecessary string casts #226

Improves SapiStreamEmitter tests, preventing unnecessary string casts #226

Conversation

fcabralpacheco commented Jan 22, 2017

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

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

Ocramius commented Jan 23, 2017

fcabralpacheco commented Jan 23, 2017

Ocramius commented Jan 23, 2017

Ocramius commented Jan 23, 2017

fcabralpacheco commented Jan 23, 2017

Ocramius commented Jan 23, 2017

fcabralpacheco commented Jan 23, 2017