Refactor SnakObjectTest

[Benestar]

Removes the strange inheritance and uses proper mocking instead.

[JeroenDeDauw]

lolwat?! :)

[Benestar]

Increased code coverage by mostly removing stuff :D

[JeroenDeDauw]

The two test classes are quite weird after this change. One tests an abstract class, and the other holds just one test for PropertyValueSnak. While there are no distinct tests for PropertyNoValueSnak and PropertySomeValueSnak. As is, I don't think this is much of an improvement over the old situation.

[thiemowmde]

This bit here confuses me. The test actually tests the mock of the abstract class, right? I don't think this is what mocks are for.

[Benestar]

The mock helps us to inject the abstract methods. We still test all methods in SnakObject not more and not less.

[thiemowmde]

The mock object is tested. The test relies on the fact that the mock object delegates unmocked methods to the original code. Which means this is actually testing parts of the PHPUnit framework.

Or to put it the other way around: This is misusing the reflection functionality of PHPUnit to execute code in a way it is not supposed to be used. This is testing code that's not even callable in production.

Either way, bad, bad design.

[Benestar]

Our testing code is not callable in production anyways because we extend PHPUnit classes everywhere. However, how was the previous design better? It used some strange kind of inheritance and abstract providers to build testing objects. Injecting the abstract methods via reflection isn't bad but helps us testing the code in the right places. Code in SnakObject should be tested in a SnakObjectTest and not in a PropertyValueSnakTest.

Furthermore, we rely on the fact that $this->assertEquals( 1, 1 ) does not fail so why not rely on this part of PHPUnit? It is clearly documented, even giving an example here.

[thiemowmde]

Some strange kind of inheritance

Really. I don't get it. This "war against inheritance" alienates me more and more. Can we (the team) please, please discuss and solve this weird controversy somehow?

I find the "run abstract methods via a mock object that does some magic reflection" approach in this pull request very, very creepy. It's just a few lines but it took me way longer than it should to get an idea of what it probably does. And I still can't believe it's actually testing what needs to be tested. So no, the previous design may have problems but this is not better.

[JeroenDeDauw]

This "war against inheritance"

You mean the "quest for good design"? :D

I find the "run abstract methods via a mock object that does some magic reflection" approach in this pull request very, very creepy.

can't believe it's actually testing what needs to be tested

Read my first comment, I fully agree with this. That is not an argument for the code reuse via inheritance approach though.

[Benestar]

As is, I don't think this is much of an improvement over the old situation.

I think this is much of an improvement because it removes lots of weirdness in SnakObjectTest like having a bunch of code just to construct a Snak instance. I see no reason why we shouldn't test an abstract class as it is and think it is better than duplicating all tests in all subclasses of SnakObject.

[thiemowmde]

I see no reason why we shouldn't test an abstract class as it is

This is not testing "an abstract class as it is". This is testing a PHPUnit mock object. We will not merge this. Please rework or abandon.