Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename our test suites so the names make sense #5940

Closed
mattab opened this issue Aug 6, 2014 · 26 comments
Closed

Rename our test suites so the names make sense #5940

mattab opened this issue Aug 6, 2014 · 26 comments
Assignees
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Milestone

Comments

@mattab
Copy link
Member

mattab commented Aug 6, 2014

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
@mattab mattab added this to the Piwik 2.6.0 milestone Aug 6, 2014
@mattab mattab added the Task label Aug 6, 2014
@tsteur
Copy link
Member

tsteur commented Aug 6, 2014

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

@mattab mattab modified the milestones: Piwik 2.6.0, Piwik 2.7.0 Aug 25, 2014
tsteur added a commit that referenced this issue Sep 25, 2014
@tsteur
Copy link
Member

tsteur commented Sep 25, 2014

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
Copy link
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
Copy link
Member

tsteur commented Sep 25, 2014

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
Copy link
Member Author

mattab commented Sep 26, 2014

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
Copy link
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
Copy link
Member

tsteur commented Sep 26, 2014

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

Those classes have been there for years, so probably yes

@tsteur
Copy link
Member

tsteur commented Sep 26, 2014

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
Copy link
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
Copy link
Member

tsteur commented Sep 26, 2014

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
Copy link
Member

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

tsteur added a commit that referenced this issue Oct 5, 2014
tsteur added a commit that referenced this issue Oct 5, 2014
tsteur added a commit that referenced this issue Oct 5, 2014
tsteur added a commit that referenced this issue Oct 5, 2014
… tests still fail and I cannot figure out why
@tsteur
Copy link
Member

tsteur commented Oct 5, 2014

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
Copy link
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 added a commit that referenced this issue Oct 6, 2014
tsteur added a commit that referenced this issue Oct 6, 2014
…moved some tests from unit to integration and explained when a test is a unit test, an integration test or a system test.
@tsteur
Copy link
Member

tsteur commented Oct 6, 2014

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?

tsteur added a commit that referenced this issue Oct 6, 2014
tsteur added a commit that referenced this issue Oct 10, 2014
Rename our test suites so the names make sense #5940
tsteur added a commit that referenced this issue Oct 10, 2014
tsteur added a commit to matomo-org/plugin-CustomAlerts that referenced this issue Oct 10, 2014
tsteur added a commit to matomo-org/plugin-VisitorGenerator that referenced this issue Oct 10, 2014
tsteur added a commit to matomo-org/plugin-CustomAlerts that referenced this issue Oct 10, 2014
@mattab mattab added the c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. label Oct 10, 2014
@mnapoli
Copy link
Contributor

mnapoli commented Oct 10, 2014

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
Copy link
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
Copy link
Member Author

mattab commented Oct 10, 2014

Helper sounds good +1

@tsteur
Copy link
Member

tsteur commented Oct 12, 2014

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
Copy link
Contributor

mnapoli commented Oct 12, 2014

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 added a commit to matomo-org/plugin-CustomAlerts that referenced this issue Oct 12, 2014
@tsteur
Copy link
Member

tsteur commented Oct 12, 2014

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
Copy link
Member

sgiehl commented Oct 12, 2014

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

@diosmosis
Copy link
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).

tsteur added a commit that referenced this issue Oct 13, 2014
…o remove duplicated code to load autoload.php and to be able to register more autoloaders (eg for test files) on demand. This I got read of many includes that had to be updated all the time and that had to be updated all the time when moving iles
tsteur added a commit to matomo-org/plugin-CustomAlerts that referenced this issue Oct 13, 2014
tsteur added a commit to matomo-org/plugin-PiwikDebugger that referenced this issue Oct 13, 2014
tsteur added a commit that referenced this issue Oct 13, 2014
tsteur added a commit that referenced this issue Oct 13, 2014
…or all declared classes so we have to make sure it is loaded
tsteur added a commit that referenced this issue Oct 13, 2014
@mattab
Copy link
Member Author

mattab commented Oct 13, 2014

Well done @tsteur closing it as it looks done and build is green 👍

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Projects
None yet
Development

No branches or pull requests

5 participants