Replace hard final with soft, annotation-based @final

[ostrolucky]

I need to depend on ResourceOutputStream, because I rely on methods missing from interface. And I need to mock it for my tests. Hence I am proposing to drop hard final and replace it with annotation based final, which is same strategy Symfony follows. This gives you best from both worlds - you are still not obligated to keep an eye for inheritance based BC issues, but now people can mock it

[kelunik]

I don't like removing the final. You mentioned the reason why you can't rely on the interfaces, but why do you need to mock the instances?

[ostrolucky]

Well would you be more comfortable with adding new interface for this? Eg. ResourceAwareStream?

Not sure I understand your question. I need to mock instance because that's what class constructor requires. It can't require InputStream interface because method I need is not there.

[kelunik]

What does ResourceAwareStream solve? Which methods do you need?

Instead of mocking the InputStream, you could use a socket pair, no?

[ostrolucky]

I need getResource method.

My tests use code like

$output
    ->expects($this->exactly(1))
    ->method('write')
    ->willReturn(new Success())
;

so expecting raw resource is not going to cut it.

[kelunik]

I think this is a rather bad test, because it tests implementation details. Instead, I suggest testing the behavior, so e.g.

public function testSomething() {
    [$a, $b] = Socket\pair();

    whenSomethingHappens(new ResourceOutputStream($a));

    thenWrittenStreamContentIs("foobar", new ResourceInputStream($b));
}

public function whenSomethingHappens(ResourceOutputStream $stream) {
    // your implementation here

    $stream->close(); // ensure the stream is closed, so buffer doesn't hang
}

public function thenWrittenStreamContentIs(
    string $content,
    ResourceInputStream $stream
) {
    $this->assertSame($content, ByteStream\buffer($stream));
}

[ostrolucky]

Well it's unit test, it's what it's meant to test. I have integration tests for other things. I need to test precisely write is triggered once only and not more. This is in order to make sure number of read and write calls is not getting out of sync. Getting same end result with different number of these calls than expected would be wrong.

[trowski]

@ostrolucky I would do as @kelunik recommended and use a socket pair to test what is written to the socket is what is expected. If you need the resource, I would also consider passing the resource to the constructor instead of ResourceOutputStream.

[ostrolucky]

I am simulating behaviour that happens when write method returns NULL, that's impossible to emulate with socket pair. You did not present any reason keeping hard final.

[trowski]

The write method should never return null.

These classes all implement an interface. There's no reason to remove final from them – you can always write another class that implements the interface, probably wrapping one of these existing classes.

[ostrolucky]

Sorry, I meant 0. Yes, I can create wrapper, but why am I forced to?