[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[jfw] Re: Private methods should not be unit-tested



On Sunday, 16 March 2014 12:31:11 UTC+1, Jindřich Pilař wrote:
>Sorry if I am bothering you with my questions

It is not bothering me at all. The contrary! I learn a lot from this discussion and hope that you find it as interesting as I do.
I just hope we are not bothering others too much with it ;-)

On Sunday, 16 March 2014 12:31:11 UTC+1, Jindřich Pilař wrote:
>What I still cant quite get is why it should be an obstacle to testing implementation.

Because publishing tests is primarily to guard, so that after changes  the software will still give the same, desired result. Changes in itself are not bad: if there is a better implementation or code is refactored to make things cleaner, clearer, better optimised or less complex, then the tests are there to guard the interface: are we doing changes within the borders of what functionallity we want to keep? If we first have to change the tests in order to change an implementation, then that test is useless. It probably was useful during development, but  what is the use of publishing a test if we first would have to change it when we are changing the software?

Good software encapsulates behaviour. That means: the interface doesn't "know" anything of the implementation. We can change whatever we want of an implementation, while the interface remains unchanged.

On Sunday, 16 March 2014 12:31:11 UTC+1, Jindřich Pilař wrote:
>Isn't removing these temporary tests a waste?


I don't think so: because they are implementation-specific they are not very useful for new implementations. They could even be a nuisance for better implementations: for you first have to decide what tests to change and what not to keep the same functionality.

On Sunday, 16 March 2014 12:31:11 UTC+1, Jindřich Pilař wrote:
> Why not have separate interface tests and separate implementation tests.


That is a very interesting idea. But might be a bit redundant too: if an implementation is not good, then a  test on an interface will fail. So, it is clear that there is something wrong. If you would have to go through much trouble to find where the implementation fails, it might be an indication that your method is too big. If you are exposing private methods (having other developers use it) then it is part of an interface and could just as well be public (or maybe protected). That might be an indication that things could be refactored to better encapsulation. The ideal is: make interfaces that don't have to change. If an interface has to change, that might be an indication that something is architecturally wrong. Those are not "rules" to make your life as a developer misirable, but to help you make better software. Better in the sense of: low coupling, high cohesion. Hence: more flexible. For change is the only constant, so things have to be changed. Part of the art of software development is building in such a way, that your software can be adapted to change, without the software that has been written to change (open-closed-principle: open for extension, closed for modification). And that is mainly applicable to the interface, not the implementation (for that can be optimised within the borders of the interface).

Maybe some tests are only useful during development (especially when you go the TDD or tests-first way). But just like a mould is useful while casting a statue, it is not a waste when the mould is not reusable. And it could only be reusable for exactly the same statue.

To be practical (and relativate this discussion a bit): in Joomla we are only talking about 4 - 6 classes where some private methods are tested. It is not a big issue.

On Sunday, 16 March 2014 12:31:11 UTC+1, Jindřich Pilař wrote:
>But for development and maintaining separate implementations i find it insufficient.

Me too! I think it is very usefull to test private mehods during development!

In the example you give you are not using a private method; the RemoteServiceUserRepository has the same UserRepository interface. You have now added a persistence-responsibility to this RemoteServiceUserRepository: it should persist its users upon destruction. If that responsibility is a specific requirement of the RemoteServiceUserRepository, then you should test that in the test of RemoteServiceUserRepository, as part of its interface. In this case I think you might also want to add that requirement to the UserRepository interface. Or, have RemoteServiceUserRepository explicitly implement something like a PersistentDestructor interface. Thinking about the example (yes I know, it is just an example), I would probably not defer something as important as persistence to a destructor, but have a controlled call somewhere to persist it (know this dilemma well, as I'm working with Doctrine ORM a lot, where persistence is "transparent", thus not part of the domain model itself and sometimes a puzzle where and when to flush() the state of the entities to the database).



On Sunday, 16 March 2014 12:31:11 UTC+1, Jindřich Pilař wrote:
>But for development and maintaining separate implementations i find it insufficient.

Me too! I think it is very usefull to test private mehods during development!

In the example you give you are not using a private method; the RemoteServiceUserRepository has the same UserRepository interface. You have now added a persistence-responsibility to this RemoteServiceUserRepository: it should persist its users upon destruction. If that responsibility is a specific requirement of the RemoteServiceUserRepository, then you should test that in the test of RemoteServiceUserRepository, as part of its interface. In this case I think you might also want to add that requirement to the UserRepository interface. Or, have RemoteServiceUserRepository explicitly implement something like a PersistentDestructor interface. Thinking about the example (yes I know, it is just an example), I would probably not defer something as important as persistence to a destructor, but have a controlled call somewhere to persist it (know this dilemma well, as I'm working with Doctrine ORM a lot, where persistence is "transparent", thus not part of the domain model itself and sometimes a puzzle where and when to flush() the state of the entities to the database).


On Sunday, 16 March 2014 12:31:11 UTC+1, Jindřich Pilař wrote:
I understand why, from the "system as a whole" we care only about interface/public methods and why not care how it is implemented internally. Interchangeability is incredibly useful thing, personally I prefer library/package approach rather than full stack framework. What I still cant quite get is why it should be an obstacle to testing implementation.

As you wrote
Of course, while developing this, you can use temporary tests, but for the system as a whole it is only important that the collection is sorted when calling the sort() method. If we also require the private methods, used in the old implementation, to pass tests, then we prevent innovation. 

Isn't removing these temporary tests a waste? Why not have separate interface tests and separate implementation tests. While requiring only the interface tests to pass. So when I am rewriting some class I can throw out implementation tests that fail and might write my own to help me while developing.

i.e.:
UserRepository with CRUD methods

// tests/interface/UserRepositoryTest.php
$user
= $userRepository->create("John", "Doe");
assertSame
($user, $userRepository->findOneByFullName("John", "Doe") );

$user
->changeLastName("Smith");
$userRepository
->update($user);
assertNull
($userRepository->findOneByFullName("John", "Doe") );;
assertSame
($user, $userRepository->findOneByFullName("John", "Smith") );

$userRepository
->remove($user);

assertNull
($userRepository->findOneByFullName("John", "Smith") );;



For any implementation this test has to pass. And that is all what package as whole needs to know that work. But for development and maintaining separate implementations i find it insufficient.
i.e. You can use some remote service to manage users and want the UserRepository to send updates to the service in one batch when destructing the object. 

//tests/implementation/RemoteServiceUserRepositoryTest.php
$service
->expect("update")->with(any())->once();
$userRepository
= new RemoteServiceUserRepository($service);
unset
($userRepository);

Now when I remove __destruct method, class is bugged. However interface tests pass anyway. Implementation one does not.
I believe this is something worth testing, but "system as whole"  doesn't care a bit when service actually saves changes.

Sorry if I am bothering you with my questions, I just lately got very interested in testing as it helps me keep my sanity while programming.

On Sunday, 16 March 2014 10:25:15 UTC+1, Herman Peeren wrote:
Jindřich, there might be some confusion in my use of the word "interface". It doesn't have to be the programming language construct of "interface", but mainly the public methods of a class in general. In your example, say we have a Class that handles some collection, and has a public method sort(). Then the class is working properly when the collection is returned sorted. We don't care in what way the sort is internally implemented. If for some reason someone uses another (better) implementation of that sort, that might use other private methods, the class should still pass the tests. Maybe the internal structure of that class is totally changed, some private methods of the old implementation are gone, some new ones in it. Of course, while developing this, you can use temporary tests, but for the system as a whole it is only important that the collection is sorted when calling the sort() method. If we also require the private methods, used in the old implementation, to pass tests, then we prevent innovation.

Your example might also be a bit unclear because it is procedural: it doesn't give a sorted version of the object, but is just a sorting-procedure that sorts an input-parameter. A sort() method as interface should be a method of some collection-object. A collection of articles could for instance have a sortByTitle() method. Here I don't care how the sorting is implemented, as long as I get a list of  articles back, alphabetically ordered by their title. Just like you can use other tires under your car as long as they are the same size (= have the same interface). Ideally we could interchange parts of our Joomla-installation more easily. That is also the idea of using Composer and putting things together. When doing so, the interface must fit; how things are done internally is of no importance, as long as the interface, the public methods work as documented.

From this thread I mainly learned that we have a lot of classes in Joomla of which developers use private methods. That being the case those private methods are actually part of the interface that is used. And that means they should be tested (and cannot be just replaced, not even by better implementations). It is interesting to see where this is the case and how we can improve this to get better decoupled software, that is more flexible because we can interchange one implementation for another more easily.

On Saturday, 15 March 2014 22:35:19 UTC+1, Jindřich Pilař wrote:
Hi,

I get the arguments, but I am having trouble to accept this white-black rule  public -> test, private -> don't.
Could't think of some "real world" example, I came up with "school" like example. Consider following pseudo code.

Interface Sorter
 array public function sort(array)

Now I would write "interface" test, which would take array (maybe random generated) use sorter and check output if is sorted.
Then I would make run all implementations of this interface trough this test.

But also if I were implementing merge sort:
Class MergeSorter implements Sorter
 array public function sort(array)
 array private function merge(array, array)

Personally I would test merge function, because the interface test would tell me "Fail, MergeSorter doesn't work". But I would have no Idea what is broken, and have to go trough entire class, read again, debug it etc.
And making merge method public doesn't make much sense to me, because it is Implementation specific to MergeSorter.
Am I wrong?

Sure, there is argument I should run tests after small changes, but I don't have to be the guy who wrote it. 
I might get to some already written code (pull request maybe) I want to merge. But between the time someone made PR and I merge it I made other changes so there came up a bug. With this interface only test I will have to find what is wrong manually, which will cost me more time.

Actually I always thought that good tests (enough quality and enough quality) is important for OSS, because it saves time (and nerves) of the guy who is in charge.


(please keep in mind this is just example I was able to quickly came up from top of my head)


On Friday, 14 March 2014 22:38:20 UTC+1, Herman Peeren wrote:
Private methods should not be unit-tested. Code quality would improve if we delete those tests.

It sounds weird and I understand it is felt as a waste to delete work you've done and lowering the code coverage, but I'll explain. Testing is not an improvement of code robustness in itself. It is done to test if the results are the same when you change the code. Change is the only constant in software development, that is why testing is important. Testing should therefore be done to an interface, not to an implementation. If you test an implementation, you are making the code more rigid, less flexible: for an other implementation could be an improvement of the code. Testing the implementation prevents change. Private methods are only necessary as part of the implementation.

In a way we have made ourselves prisoners of the "code coverage" measurement. It is easy: we just look at a number and are more satisfied if it is in the direction of 100% and sad if it is in the direction of zero. Colors in the tools we use support that: the more 100% = the more green = good and happy. Zero = red = wrong. The tools and measurements get a life on their own. We almost forget that the tools there are to help us, not the other way around.

I mainly came to this conclusion after watching "The Magic Tricks of Testing" presentation by Ruby star Sandi Metz; see for instance https://www.youtube.com/watch?v=qPfQM4w4I04 and https://www.youtube.com/watch?v=URSWYvyc42M
She points out that we should only have assertions for incoming queries and commands, asserting the result of an incoming query and the direct public side effects of an incoming command. For the outgoing commands we should only test the expectation that they are sent. We should ignore all tests of messages sent to self (they test the implementation , not the interface) and outgoing query messages (they are redundant). Testing private methods can be usefull during development (certainly if you follow a TDD road), but should be deleted afterwards. Or, funny moment in Sandi's presentation: be marked as "delete these tests if they fail".

Some other arguments against testing too much can be found in a recent paper by James Coplien ('Cope') "Why Most Unit Testing is Waste" : http://www.rbcs-us.com/documents/Why-Most-Unit-Testing-is-Waste.pdf

During the coming GSOC (Google Summer of Code) several students apply to work on more Unit Tests for Joomla. The main measurement for success is "code coverage". I hope we will get some better criteria in. Maybe we can even improve some code by having less testing code.  

I looked for tested private methods in the Joomla Framework and fortunately found not much matching that criterium:

Private methods that are currently tested in the Joomla Framework (only in 4 test-files):
  • Joomla\Application\Cli\ColorProcessor::replaceColors
  • Joomla\Cache\File::checkFilePath
  • Joomla\Cache\File::fetchStreamUri
  • Joomla\Cache\File::isExpired
  • Joomla\Oauth1\Client::_generateRequestToken
  • Joomla\String\Inflector::addRule
  • Joomla\String\Inflector::getCachedPlural
  • Joomla\String\Inflector::getCachedSingular
  • Joomla\String\Inflector::matchRegexRule
  • Joomla\String\Inflector::setCache

The private methods Joomla\Archive\Zip::readZipInfo and Joomla\Archive\Zip::getFileData are marked as covered in the test of the calling Joomla\Archive\Zip::extractCustom , which could be a way to get more coverage.

This is the other way around, marking the private methods as
$this->markTestSkipped('This method is tested via requesting classes.');
  • Joomla\Linkedin\Groups::_likeUnlike
  • Joomla\Linkedin\Groups::_followUnfollow
  • Joomla\Linkedin\Stream_likeUnlike

In order to preserve our "good feeling" about code coverage, we could adopt a standard way of marking private methods as covered. Or just take the code coverage as measure with a grain of salt, in favour of better judgement for the need and desirability of tests.


--
Framework source code: https://github.com/joomla/joomla-framework
Visit http://developer.joomla.org for more information about developing with Joomla!
---
You received this message because you are subscribed to the Google Groups "Joomla! Framework Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to joomla-dev-framework+unsubscribe AT googlegroups.com.
Visit this group at http://groups.google.com/group/joomla-dev-framework.