[#23962] - [4.0] Migrate unit tests (back) to main repo
- Closed
- 22 Feb 2019
- Medium
- Build: 4.0-dev
- # 23962
- Diff
- Hackwar:testmigrate
User tests: Successful: Unsuccessful:
History
About a year ago the unit tests for Joomla 4.0 have been moved from the main repository into its own repo in https://github.com/joomla/test-unit. The reasoning behind that was to have this well capsulated and to prevent people from messing around with unit tests to make their broken code pass our system.
Furthermore the unit tests were split up into two repositories, keeping one as pure unit tests and the other as integration tests. This in theory was also a good idea, since it allows us to run the tests somewhat in parallel and it makes the testing setup easier, since the unit test setup wouldn't need a database available.
Issues
Unfortunately these changes had and have grave consequences which is why this PR tries to revert this (in part).
The first issue is, that when migrating the tests, only a few were made runable with the new system, resulting in currently only 123 tests being run instead of the 5000+ tests we have for 3.x. The old tests right now don't execute at all on the new codebase.
The next and even more important issue is, that no one wrote any new tests and new code was commited without any supporting unit tests.
Another issue: The integration tests were never made runable and never ran on the current codebase. They are not part of our drone setup right now.
Last but not least, the split setup for the unit tests also means that we can't collect statistics like the code coverage for 4.0 right now.
Proposal
This is why I propose to move the tests back into the main repository. I also strongly believe that we should drop the seperation between unit and integration tests, simply because the distinction is quite difficult to make and would result in long discussions instead of improving our testing codebase. This would (again) allow us to also collect statistics like the code coverage statistics that we have for 3.x.
This now moves over the content of the test-unit repository and updates all references to the new location. This allows people to start writing tests again and would also allow us to (eventually) enforce the requirement of unit tests for a PR again. With the new location there also should be more interest by people to write tests. We can then migrate the old 3.x tests over to this and update them. The old repository would be obsolete with this PR. We should review the open PRs and then archive or delete it. Eventually the javascript and system tests should be moved over similarly.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Unit Tests External Library Composer Change Repository |
| Labels |
Added:
?
?
?
|
||
I agree that we should move the tests back.
I do not agree that we should drop separation between unit and integration tests.
We should also move js and system back to core and probably some of the helpers like joomla browser.
Anything that can be useful as a resource away from the main CMS repo should continue to be separate from the repo. https://github.com/joomla-projects/joomla-browser is one of those tools, unless you're suggesting that for an extension to be able to run automated tests against the CMS they must run against the git clone (which I personally don't see the need to go this far with, automated testing of extensions should be possible by downloading a release package as well; the software's tests though should never live in a remote location from the software they are testing).
The problem with joomla-browser is that it contains core locators. So if you change code that affects those locators, you have to change an external package to make your pr pass. Once your changes to joomla-browser get merged, all other open PRs in the main repo will fail until your PR in main repo gets merged too. Thats just a bad workflow and we need to find a better solution.
We just had such a case in #23942 and probably will have some more in the near future, when admin theme changes get pushed to main repo.
possible workarounds:
- force pr authors to change composer.json to load their patched version of joomla-browser (bad!)
- implement possibility to override locators from joomla browser in joomla core... (also bad, because it makes joomla browser petty much useless in the long term, nobody will sync overrides and joomla browser).
- ???
But thats topic on its own... im very fine when we keep joomla browser as a separate package for now and start moving tests back to core.
I'm happy to provide PRs for the other tests, too. But please merge this one first. I have little interest in doing unnecessary work and would want s commitment from the project to go this way.
Regarding merging unit and integration, I don't see any benefit for that separation. The fact alone that developers won't know if the class should be tested as unit or integration test will keep them from writing and tests at all. And even if they write tests, you will then have discussions in the PR which kind of test this is. See my PR in the unit test repo as example.
Last but not least, we are running us of metrics like code coverage.
The fact alone that developers won't know if the class should be tested as unit or integration test will keep them from writing and tests at all.
A class should not have separate types of tests. Integration tests imply testing how something works with other parts of the system, which implies multiple units (classes) are involved. Unit tests imply a single unit (class) is being tested. If you're unit testing Joomla\CMS\Document\Document, that does not mean it cannot or should not also have integration tests. And just because the class is covered by integration tests does not mean it cannot or should not be unit tested. And as with every rule, there are exceptions; packages like the database API are things that are best covered with by integration tests against a live database for most of its functionality (the query classes should be able to be done with unit tests without a live database connection though as if you can't validate the classes build valid SQL statements without a database connection there are probably other architectural flaws in that API).
The problem with joomla-browser is that it contains core locators. So if you change code that affects those locators, you have to change an external package to make your pr pass. Once your changes to joomla-browser get merged, all other open PRs in the main repo will fail until your PR in main repo gets merged too. Thats just a bad workflow and we need to find a better solution.
That is an issue with any testing resource reliant on a specific template structure. That is not an issue with the tool itself.
Adding on to the whole unit versus integration versus functional versus whatever test type bit...
In my best tested project, there are functional (integration) tests for some classes that are best tested with a live database connection (complex internals that are best tested with real data and not mocks), and my controllers have functional tests where the request/response workflow is tested using the framework's test utilities. Those controllers that are functionally tested call several services generally to validate an end-to-end workflow. Many of those services (classes) also have unit tests covering their internals and for the most part go more in-depth than what I'm doing with the functional tests (my unit tests usually cover all of my error scenarios, whereas my functional tests for the controllers are covering the success paths or select error paths from direct dependencies and not validating that the controller catches and handles 4 or 5 different possible error scenarios from the transient dependencies of the services it uses).
That is an issue with any testing resource reliant on a specific template structure. That is not an issue with the tool itself.
sry, not getting the point. joomla browser does heavily rely on template structure... that is the issue im pointing out. if you remove template related stuff from joomla-browser you basically end up with an codeception module that does know something about the local joomla environment (=== config file)
what do you suggest regarding joomla browser?
The fact alone that developers won't know if the class should be tested as unit or integration test will keep them from writing and tests at all.
if code is covered by integration it does not mean, that there shouldnt be any unit tests and vice versa. combined coverage reports would be missleading.
And even if they write tests, you will then have discussions in the PR which kind of test this is.
cant guarantee that there will be no discussions, but you can easily see the difference between unit and integration test. when a unit test affects outside world... its not a unit test. i think that is clear and thats not the real problem. the real problem is that in some environments (like joomla) it is sometimes hard to write real unit tests... we should not mix things up here and merge the two test suites just because of that.
Last but not least, we are running us of metrics like code coverage.
separated coverage reports are much more useful, than a report that combines unit and integration tests
Meh, I disagree, but whatever. Right now the discussion is mute anyway, since we have no integration tests.
Totally happy with this too. Only question I need to figure out is this going to cause conflicts when merging up the J3 code or cause it's a merge commit i assume it will be ok?
I honestly can’t answer that, I can’t think of a situation where I’ve
deleted and re-added files on a branch at the same path and had to merge
branches later. For the most part you’ll take the 4.0 version anyway.
On Thu, Feb 21, 2019 at 6:03 PM George Wilson notifications@github.com
wrote:
Totally happy with this too. Only question I need to figure out is this
going to cause conflicts when merging up the J3 code or cause it's a merge
commit i assume it will be ok?—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#23962 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoVcKUNUnf-SsQL9BdNmnsBQbUycFks5vPzO2gaJpZM4bGSc9
.
--
- Michael Please pardon any errors, this message was sent from my iPhone.
Presumably the tests were removed to their own repos at the request of the Automated Testing team. Its hard to know if they still exist as their have been no reports for 7 months but assuming they do exist shouldnt this be a decision that is up to them
@brianteeman the testing team has been discussing this for a very long time and they have been informed about this PR.
This is an ongoing discussion within the testing team.
The requirement (we had long time ago) to only be able to merge a new feature when it is backed up with test lead to a situation that people who where to a certain extend able to develop a feature had the problem to write test for the feature. This resulted in tests with a very low quality and often to test that are testing NOTHING. If this test had been used as a blueprint for new test we ended up with more shit.
First and foremost we need a quality management for tests! Our idea to move the test was to allow the quality management for testing. The downside is that the over all workflow is more complicated. Since we have code owner and other ways to allow better quality management, we started thinking about how to move tests back. What doesn't makes any sense at all is that we are merging the bad quality back without looking at it.
So here is the plan:
We are starting with moving the unit tests back to the main repo. We are only moving pure unit tests back, that means classes that we can test in isolation. We are also going to reorganize them (namespaceing when possible). The unit test will be executed with phpUnit and not codeception. We don't see an advantage with using codeception even if we would have than only ONE tool for testing.
This task should not take longer than a few days so I am closing this PR, a new one is coming soon.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-02-22 10:53:26 |
| Closed_By | ⇒ | rdeutz |
I disagree with closing this PR, with the justification and with the way this is communicated. I doubt that anything will happen with the tests, they had a years time for that. I will re-open this PR in a month, unless something substantially has happened since then. Moving the bootstrap.php file into the main repo again without any tests is not a substantial change. BTW: Closing this PR has not been discussed in the Testing Glip group.
This. 1000x this.