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
Comments
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 |
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 |
My thoughts/opinions:
|
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. |
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? |
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. |
Those classes have been there for years, so probably yes |
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. |
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. |
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. |
Ok, I understand the benefits of this change, I see no problems w/ it ;) |
… tests still fail and I cannot figure out why
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. |
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. |
…moved some tests from unit to integration and explained when a test is a unit test, an integration test or a system test.
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? |
Rename our test suites so the names make sense #5940
… test namespace
…ed with other tests
I see there is now a |
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. |
|
Didn't know what |
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.
|
They are in a new "namespace" to be able to stay backwards compatible and to ideally have less mess under |
Imho the files should be moved to the folders they belong to, where |
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). |
…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
…or all declared classes so we have to make sure it is loaded
Well done @tsteur closing it as it looks done and build is green 👍 Epic task of renaming and re-organising... we appreciate a lot! |
The goal of this ticket is to rename our test suites to make them more consistent with software engineering practises.
Rename:
Rename in:
travis.yml
Steps:
The text was updated successfully, but these errors were encountered: