@mattab opened this Issue on August 6th 2014 Owner

The goal of this ticket is to rename our test suites to make them more consistent with software engineering practises.

Rename:

  • CoreTest -> UnitTest
  • DatabaseTest -> IntegrationTest
  • IntegrationTest -> SystemTest

Rename in:

  • in test code in core
  • in all plugins
  • change in travis.yml
  • change in phpunit.xml
  • etc.

Steps:

  • Rename files and folder in core
  • Update tests in all core plugins
  • Update tests in pro plugins
@tsteur commented on August 6th 2014 Owner

SystemTest could be AcceptanceTest as well but I think SystemTest would describe it better in our case. Here are some links about this topic http://stackoverflow.com/questions/4904096/whats-the-difference-between-unit-functional-acceptance-and-integration-test and http://www.buzzle.com/articles/integration-testing-vs-system-testing.html

@tsteur commented on September 25th 2014 Owner

I just renamed the Test classes IntegrationTestCase => SystemTestCase, DatabaseTestCase => IntegrationTestCase and then wanted to provide a UnitTestCase that only extends PHPUnit standard one.

Then I noticed this will break all tests in all 3rd party plugins. So I was thinking the only way to use better naming would be to place them in a new directory (another namespace) and leave the old classes there but the old classes would extend the new ones to not having to maintain them. It would be maybe still confusing since some 3rd party plugins are already updated and some not and one never knows whether an IntegrationTestCase is now an actual integration test case or a SystemTestCase. Any opinions? cc @mattab @diosmosis

@diosmosis commented on September 25th 2014 Member

My thoughts/opinions:

  • I think UnitTestCase should be named PiwikTestCase and should set up a minimal Piwik environment (ie, everything that won't affect speed, so, for example, plugins loaded, but no database created). I don't think it makes sense to create a class that does nothing but extends the PHPUnit test case, seems pointless to me.
  • I would prefer for there to be one base test class instead of 3 (or more). That isn't to say I want every test to create a database just in case it might use it, but I think if an easy facade for specifying exactly what kind of environment a test needs is devised, one base class will be easier to use than three.
@tsteur commented on September 25th 2014 Owner

Would be cool to have only one kind of testcase and be able to bootstrap database or whatever needed from there but I think it is out of scope of this ticket where it is basically only about renaming to fix our current naming which is misleading. This would be the next step then. Probably it won't happen too soon so it might be still worth doing this here.

I would not always bootstrap things like plugins as it is still slow (I'd like to execute all unit tests in < 5 second) and for me personally it is quite important in tests to be aware which dependencies a test has. Otherwise you might not even notice some dependencies, especially when writing unit tests. It should be still very! easy to bootstrap certain things like loading all plugins, database, ... with one or two standard method calls if needed.

@mattab commented on September 26th 2014 Owner

Then I noticed this will break all tests in all 3rd party plugins.

Are there other plugins not Piwik/Piwik PRO maintained that include tests? if No, then we only break Piwik/Piwik PRO tests... so I vote +1 to rename tests classes and break any existing tests. We can document in the developer changelog how one could rename their test classes to keep test compat with Piwik 2.8.0. The benefit is that all our test code will be consistent and we don't need the bridge class solution waiting for deprecation. What do you think?

@diosmosis commented on September 26th 2014 Member

I would not always bootstrap things like plugins as it is still slow

Is it slow? I didn't notice anything personally, but I don't use a VM so maybe that makes them faster. You're right that plugin loading should be explicit, but that doesn't necessarily mean that the UnitTestCase/PiwikTestCase class shouldn't do any Piwik specific setup. If a developer doesn't want to have the Piwik environment setup (w/ I guess at least Config::getInstance()->setTestEnvrionment()), they can inherit from phpunit's testcase class.

@tsteur commented on September 26th 2014 Owner

Are there other plugins not Piwik/Piwik PRO maintained that include tests?

Those classes have been there for years, so probably yes

@tsteur commented on September 26th 2014 Owner

Is it slow?

Core tests take even 3 minutes on Travis --> That's slow in my opinion. I'd prefer a result in seconds :) If a unit test requires a $config->setTestEnvironment() it is not a unit test, rather an integration test. Don't get me wrong. I'm not saying we should write unit tests for everything. For sure not.

@diosmosis commented on September 26th 2014 Member

I'm not arguing about semantics, I don't care if a test is an integration test or a unit test or whatever. I care about not wasting my time w/ fixing regressions or figuring out why my test environment isn't being setup properly. For example, when writing this test: https://github.com/piwik/plugin-LoginLdap/blob/35_ldap_user_conversion/tests/Unit/UserSynchronizerTest.php I had to waste 3+ hours trying to figure out why UsersManager\API::setSingletonInstance() wouldn't work. And in a test that dealt w/ components (ie Report/Dimension/etc.) I had to waste an hour or two realizing that Plugin\Manager::unloadPlugins() doesn't actually unload the plugins. This happens all the time for me and having accurate naming of test base classes won't help at all. Having some test class/utility that automatically and without error sets up a test environment for me would be very helpful. As would refactoring Piwik to be easier put into testing mode.

Of course, it begins to dawn on me that this is not strictly relevant to this ticket ;) If you agree or want to discuss further though, I'll create a new ticket for this.

You should note w/ travis that ~1.5 mins is travis setup time. If I remove HttpTest and ServeStaticFileTest they run in 18.35s on my machine.

@tsteur commented on September 26th 2014 Owner

I totally agree with you. I am often wasting days! As said it is just not relevant to this topic.

I care much about the naming as we allow other developers to write tests for their plugins where they are confrontend with our naming which is simply wrong and very confusing. Imagine explaining this in a blog post ;) I guess you know what I mean g. It's like using a Singleton pattern but naming it Composite or Observer pattern.

@diosmosis commented on September 26th 2014 Member

Ok, I understand the benefits of this change, I see no problems w/ it ;)

@tsteur commented on October 5th 2014 Owner

I can't make the test work on Travis when breaking BC unless I create a branch for each plugin / submodule as well :( will have a look how hard it is to stay BC

I just started to move files into correct folders (Unit, Integration and System instead of Core, Plugins and Integration which made no sense) and renamed test suites to UnitTest, IntegrationTest and SystemTest as it should be (UITest stays the same as it makes sense). As mentioned the previous suites and folders didn't make so much sense. Also the groups were sometimes not correctly used. The groups should be like "core", "plugin", "$pluginName" etc. So we can run either for instance a suite "UnitTest" (which includes all unit tests of core and plugins) or we can run the core tests (which includes all core Unit, Integration and System tests) depending on what we need to know. It is then also possible to say "run all unit tests but only the ones for all plugins or only the ones of a specific plugin" etc.

In general there is still something wrong with the tests as @diosmosis already mentioned. There should be like only one base PiwikTest class and the test can decide what to setup and when. Depending on what it is the test should then be placed in a directory Unit, Integration, UI or System. This will solve many logical problems and solve some things that are very confusing. For example if a CommandTest needs a Database it is hardly possible since it cannot extend CommandTestCase and IntegrationTestCase (to be fair a CommandTest should not need a DB as all the logic should not be in a command itself but in another class). It is confusing for instance because a test might be an Integration or SystemTest although it does not extend the related base test class.

There is another problem with the test files in the plugins see https://github.com/piwik/piwik/blob/5940_testRenamingAndCorrectFolders/tests/PHPUnit/phpunit.xml.dist#L37 As you can see all tests in a plugin are currently assumed to be integration tests when they can be actually unit, system, ui or integration. As a plugin should represent similar/same structure as core (in this case as tests/PHPUnit) a plugin should have the folders tests/Unit, tests/Integration, tests/System, ... as well.

@diosmosis commented on October 5th 2014 Member

Just thought of something, I think after my changes to DatabaseTestCase, it might be possible to merge DatabaseTestCase & IntegrationTestCase completely. I'm not sure if this helps, but it seemed worth mentioning.

@tsteur commented on October 6th 2014 Owner

It would be a start. Next step could be like methods to decide what to setup and when to setup (before each class or each test etc). Do we have already a ticket for this?

@diosmosis commented on October 6th 2014 Member

Do we have already a ticket for this?

For the single test base class? I didn't create one, but I can if you like.

Btw, since all UI tests are done in JavaScript now there shouldn't be a UITests suite or base class. If there is I forgot to remove it.

@tsteur commented on October 6th 2014 Owner

Would be great if you create an issue and will remove the UI tests suite

@diosmosis commented on October 6th 2014 Member

@tsteur Created an issue here: https://github.com/piwik/piwik/issues/6387 Feel free to edit it.

I've removed the UITests suite in master as well.

@mnapoli commented on October 10th 2014 Member

I see there is now a Piwik\Tests\Impl namespace in which several classes like Fixture or ApiTestConfig have been moved. Would it make sense to name that new namespace Piwik\Tests\Helper? I don't see the meaning of Impl?

@diosmosis commented on October 10th 2014 Member

Short for implementation. I've seen it used in Java & C#. For C++, I've seen 'Impl' or 'detail' depending on how OOP the code is. I don't know what the convention for PHP is.

@mattab commented on October 10th 2014 Owner

Helper sounds good +1

@tsteur commented on October 12th 2014 Owner

Didn't know what Impl was for either but it was used already for other classes there and thought it was Implementation. Helper makes no sense to me as all classes are helpers. It's like naming something $data or $info. PHPUnit is dividing its classes in Extensions, Framework, Runner which could be an option but Framework or Extension is not really so much more meaningful to me as well. On the other side reusing a similar structure can be useful once also providing also our own Asserts, Filters, etc.

@mnapoli commented on October 12th 2014 Member

Yes what I meant was I don't see the meaning of Implementation since none of those I've opened implemented an interface. At best some are abstract classes ;)

If we wan't to get rid of that abstract naming, maybe we need to get rid of that namespace at all.

  • Fixture (base fixture class) -> move to Piwik\Tests\Fixtures
  • IntegrationTestCase -> move to Piwik\Tests\Integration
  • SystemTestCase -> move to Piwik\Tests\System
  • ApiTestConfig, TestRequestCollection and TestRequestResponse -> move to Piwik\Tests\System since it's only used by SystemTestCase
@tsteur commented on October 12th 2014 Owner

They are in a new "namespace" to be able to stay backwards compatible and to ideally have less mess under tests/PHPUnit one day. Don't think "implementation" was meant in terms of "implements an interface" here. @sgiehl I think we used to put them all in an Extensions folder in other projects? I quite like this idea since it follows PHPUnit structure. Developers are kinda used to this kind of naming. All the TestCases could go into Extensions. The TestRequest classes into Framework/TestRequest. Our own assertions could go into Framework/Assert (once we have some => BTW: we should create some to remove duplicated code). It would make it easier as PHPUnit already provides some kind of structure for us and also uses this itsel for other components like "phpunit-mock-objects".

@sgiehl commented on October 12th 2014 Member

Imho the files should be moved to the folders they belong to, where
possible. For the other stuff Extension should be fine.

@diosmosis commented on October 13th 2014 Member

I think tests/PHPUnit/Framework/TestCases for different test cases, and tests/PHPUnit/Framework/Asserts for asserts (includes ApiTestConfig + others since they are only there for a type of assert). Framework could be exchanged w/ Extension(s).

@mattab commented on October 13th 2014 Owner

Well done @tsteur closing it as it looks done and build is green :+1:

Epic task of renaming and re-organising... we appreciate a lot!

This Issue was closed on October 13th 2014
Powered by GitHub Issue Mirror