4

Write tests to coverage 100% code is something we should attempt to achieve. But I came up with situaction where I don't know how to test method (factory method):

public function getDocument(){
    $document = new Document();
    $document->settings(new Settings());
    $document->filesystem(new Filesystem('e:'));
    return $document;
}

Purpose of that method is shortcut to create document, without everytime write 3 lines.

How to test this method?

Or maybe this is situation why we have @codeCoverageIgnoreStart block? Exactly for that reason PHPUnit provide this kind fo annotations.

EDIT: The main idea behind this method is make client life easier. Nothing more, no configuration etc.(but the method will be good place to do it).

//I don't want bother client with Settings() and Filesystem('e:')
$document = new Document(new Settings(), new Filesystem()); //NO
$document = Files.getDocument() //much easier and shorter.

//Changing API to getDocument($var, $var) make no sense, the same thing I could have normally.
$document = new Document(new Settings(),new Filesystem('e:'));

Maybe I should thing about if I really should provide that method, user who want use document should know of dependences, it shouldn't be hide.

Sonny D
  • 897
  • 9
  • 29
  • 1
    What's problem? For this method need test that document have definite settings and filesystem. – Onedev_Link Sep 12 '15 at 16:06
  • Sure, I now that. But in unit testing we shouldn't rely on dependencies, so test should pass even if Settings or Filesystem class is not defined yet. – Sonny D Sep 13 '15 at 12:03
  • @tne says right answer. Read about depency injection [in this article](http://www.martinfowler.com/articles/injection.html). – Onedev_Link Sep 13 '15 at 12:09

4 Answers4

3

Inject your dependencies (Document, Settings, Filesystem) via the constructor, then use test doubles as appropriate.

Also reconsider your 100% coverage policy, it's definitely not clear that it's actually a good thing.

tne
  • 7,071
  • 2
  • 45
  • 68
  • Okay I get what you mean, this will allow me test method. But I need new instance every time when I create document. – Sonny D Sep 13 '15 at 12:04
3

What this method does? Returns initialized Document object. So all you have to verify is that the returned value is a Document instance and that it has Settings and Filesystem objects set. Easy if you have getters for those, otherwise you have to access the respective properties.

The test may sound very basic, but it does test what it needs to. When you refactor your code in a way that the settings and filesystem are injected, the test will still tell you if the document has those properties set at all.

It's called unit testing because you are testing a unit, not an object or a method. If your unit has multiple classes, let it be. There's no need everything to be injected and there's no need everything to be mocked - those things ease testing, but in certain cases it's even better not to mock them

Maxim Krizhanovsky
  • 26,265
  • 5
  • 59
  • 89
  • I skip test this method. But probably I will use assertInstanceOf. I don’t have getter for Settings Filesystem, because they shouldn’t be accessible from Document. To check it I will use ReflectionClass.To the second part, I don't know...in my thinking about unit testing (can be wrong), we shouldn't use original implementation of dependencies (and that’s I want to get rid), all should be mocked(stubbed, faked). We testing ‘unit’, but must do it in separately environment, which we can easy configurable. Test this particular 'unit' should pass without matter of anything outside testing ‘unit’. – Sonny D Sep 14 '15 at 12:27
  • 3
    @SonnyD I suggest you to read [Test Smell: everything is mocked](http://www.mockobjects.com/2007/04/test-smell-everything-is-mocked.html) by Steve Freeman (or even better - his book Growing object oriented software, guided by tests) – Maxim Krizhanovsky Sep 14 '15 at 12:41
  • This is (truly) great advice. I think the question of whether to mock and the question of whether to invert dependencies (e.g. injection) are entirely distinct. I suggest OP evaluates the API to decide if they want to put it behind adapters or not (recommended if the API can be minimized, otherwise using a mock library is probably more appropriate as long as the interface is stable and minimal). I agree that dependencies shouldn't unnecessarily be injected if they're truly value objects with no or very "static" ctor args. In this case OP would want to at least inject the FS ctor parameter. – tne Sep 14 '15 at 15:41
  • Okay, I will try read book you suggest. Example form your link is similar to my problem (Settings class). This answer is most thought-provoking for me. But... finally I just leave this method untested. – Sonny D Sep 15 '15 at 09:10
1

Pass the dependencies to the factory method, initialize the new object inside, and configure it properly. In the test, the dependencies will be mocks instead of real objects.

method 1

Pass factories that allow to create the dependencies:

public function getDocument(SettingsFactory $sf, FilesystemFactory $ff){
    $document = new Document();
    $document->settings($sf->getSettings());
    $document->filesystem($ff->getFilesystem());
    return $document;
}

In the test, you should:

  • create Settings instance or mock and a SettingsFactory mock that expects one call to getSettings and will return the Settings instance
  • create Filesystem instance or mock and a FilesytemFactory mock that expects one call to getFilesystem and will return the Filesystem instance
  • call the DocumentFactory method, passing the factories. Check that a Document object is returned
  • check that the objects assigned to Document are the same that you configured the mocks to return

A variant on this is having the getSettings and getFilesystem as methods of the Document factory. In that case you should create a partial mock of the Factory, and set the expectations on it. So the real getDocument method is called, but when getSettings and getFilesystem methods are called, you return controlled instances.

method 2

Pass the actual dependencies:

public function getDocument(Settings $settings, Filesystem $filesystem) {
    $document = new Document();
    $document->settings($settings);
    $document->filesystem($filesystem);
    return $document;
}

In the test, you should:

  • create Settings instance or mock
  • create Filesystem instance or mock
  • call the DocumentFactory method, passing the Settings and Filesystem. Check that a Document object is returned
  • check that the objects assigned to Document are the same instances that you passed to the factory method
gontrollez
  • 6,372
  • 2
  • 28
  • 36
  • Your idea is good and I understand it. Thanks for writing answer, generally your solution is similar to those form comments I links: (do this again: http://stackoverflow.com/a/7763207/2490611 ). I edited my question, please see it. – Sonny D Sep 14 '15 at 12:24
  • Dependency injection can be managed by using some library: https://github.com/ziadoz/awesome-php#dependency-injection – gontrollez Sep 14 '15 at 12:39
  • @SonnyD If you think the answer solved your problem, then you should accept the best answer as a courtesy, but also to help future viewers of this question know what the proper solution was. – Steven Scott Sep 14 '15 at 14:07
-2

Kind of I find answer: this code isn't testable.

Wherever you have new XXX(...) in a method under test, you are doomed.

More: https://stackoverflow.com/a/7763207/2490611

Community
  • 1
  • 1
Sonny D
  • 897
  • 9
  • 29