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

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

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.

UserRepository with CRUD methods

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

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


($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. 

= new RemoteServiceUserRepository($service);

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:

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.