Skip to content

Conversation

legionth
Copy link
Contributor

@legionth legionth commented Feb 9, 2017

The stubs aren't needed in the tests and can be replaced by a simple Connection mock.

This will make it easier to create tests for an upcoming PSR-7 PR.

->withConsecutive(
array($this->equalTo("HTTP/1.1 200 OK\r\nX-Powered-By: React/alpha\r\nTransfer-Encoding: chunked\r\n\r\n")),
array($this->equalTo("0\r\n\r\n"))
);
Copy link
Member

Choose a reason for hiding this comment

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

LGTM, but doesn't match with the test title anymore?

public function testRequestEventIsEmitted()
{
$io = new ServerStub();
$socket = new Socket($this->loop);
Copy link
Member

Choose a reason for hiding this comment

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

How about using a mock here? (refs #93)

See also below.

@clue clue added this to the v0.4.3 milestone Feb 9, 2017
@legionth
Copy link
Contributor Author

legionth commented Feb 9, 2017

Updated. Checkout the newest commits.

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@WyriHaximus WyriHaximus requested a review from jsor February 9, 2017 17:37
Copy link
Member

@jsor jsor left a comment

Choose a reason for hiding this comment

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

Pesonally, I'd have preferred factory methods for creating the mocks, but i'm good with either.

@WyriHaximus WyriHaximus merged commit 51be115 into reactphp:master Feb 9, 2017
@legionth legionth deleted the remove-stubs branch February 16, 2017 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants