Review changes for phpunit 8

[phil-davis]

PR #37307 implemented phpunit version 8. Some test and code changes were needed.
Review to see if they can/should be refactored differently.

1. Provide a default constructor for OC\Files\Storage\Common 5d8dcbd

[PHP Fatal error:  Class Mock_Common_a6b4666b contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (OCP\Files\Storage::__construct) in /drone/src/lib/composer/phpunit/phpunit/src/Framework/MockObject/MockClass.php(42) : eval()'d code on line 3]

Maybe something different could be done to fix that?

2. Adjust getting protected and private object properties in unit tests 6785277
   There were some unit tests that "looked inside" objects and checked the values of private and protected properties. The tests were refactored to get those by casting the objects to array and looking in the array - that is a tricky way to "see inside" an object.

Actually unit tests should not really be "looking inside" an object - they should be testing the external behavior of the public methods and properties. Review the things in that commit and think "why is that being tested in the first place?", "what should really be done?"

[phil-davis]

@VicDeo I made somewhat-hacky changes to the unit tests so that phpunit 8 could be merged. That will get phpunit 8 into the daily-master-qa tarball tonight and make it easier to progress with finding app issues with PHP 7.4 + phpunit 8 tomorrow.

Maybe you could look at the commits mentioned above, and see how it could be refactored better?

[jvillafanez]

Provide a default constructor for OC\Files\Storage\Common

I don't find a good reason to have the __construct(...) method in the interface. The interface should care about how the implementations must behave, but not how they have to be built.
The problem is that the current architecture requires such restriction because it's core the one creating the storage objects and it needs a common method
It seems better to register a factory somewhere and let that factory be in charge of creating the storage instance. Basically something like

$globalFactoryStorage->registerFactory('SMB', new SMBFactory(....));
.....
$factory = $globalFactoryStorage->getFactoryFor($storageType);
$storage = $factory->getStorageWithParams($parameters);

It might be interesting to explore something like this, but this is quite a big architectural change to do and unlikely to be implemented.

Alternatively, 5d8dcbd is a viable workaround without any important change. I guess we'll keep that change.

Actually unit tests should not really be "looking inside" an object - they should be testing the external behavior of the public methods and properties. Review the things in that commit and think "why is that being tested in the first place?", "what should really be done?"

I Completely agree. I'll check alternatives (I don't think we want to remove the assertions or the whole test)

[jvillafanez]

• testParseRetentionObligation (apps/files_trashbin/tests/ExpirationTest.php):

  • remove the test. The code should be implicitly fully covered by EACH of the public functions using that one.
  • use a different class with a public method, and inject the class there.
  • test it by creating a new public getState() method to expose the information.

  I'd vote for the first one. The test data used by the tests for getMaxAgeAsTimestamp and isExpired method should covered all the possibilities.

• Same alternatives for apps/files_versions/tests/ExpirationTest.php (it seems there are missing tests there)

• testNewClientWithProxy and testNewClientWithProxyAndAuth (tests/lib/Http/Client/WebDavClientServiceTest.php)

  • remove the assertion and check only that a client is returned (cannot check anything more)
  • include a getUsedConfig() in the client instance so we can check the configuration being applied to the created client. Kind of questionable because this will be used only for testing without any additional purpose.

• tests/lib/SystemConfigTest.php

  • mock the injected config object and ensure the method is called with the proper parameters. The SystemConfig class doesn't have any other responsibility (at least within the methods touched)

• testConstructor (tests/lib/Template/ResourceLocatorTest.php)

  • remove it. Nobody tests constructors

That's my analysis

[jvillafanez]

#37362 should improve the situation.

The WebDavClientServiceTest remains unchanged for now. I'm sure we should verify the objects created by the ClientService are properly configured, but I think we'll need code changes to do so. Removing the assertions might not be a good enough solution.

[phil-davis]

Done in #37362
IMO that is enough refactoring here.