joomla-cms
joomla-cms copied to clipboard
joomla
Facilitate running tests
Summary of Changes
- Move ldap tests to integration, so no configuration (nor docker or any other service) is necessary for unit testing
- Improve/Complete the READMEs on running tests as promised
- Make ldap server configuration configurable
- Add 127.0.0.1 to certificate
Testing Instructions
Following the READMEs:
- Run the unit tests without configuration
- Run the integration tests with your own ldap server
Actual result BEFORE applying this Pull Request
You spend a lot of time figuring out how the CIs are doing it and copy the same behavior.
Expected result AFTER applying this Pull Request
It took you less time and maybe even tested on your existing ldap server.
Documentation Changes Required
Point people to these READMEs
@Hackwar I can skip the ldap tests during the Integration tests if JTEST_LDAP_HOST in the xml config is empty, but I did not implement it because that results in false successful integration tests.
Hey @tatankat, thanks for this work. I'm going to merge this as soon as possible, however I would ask you to indeed mark the tests as skipped as described here https://phpunit.readthedocs.io/en/9.5/incomplete-and-skipped-tests.html if LDAP tests are not explicitely expected to run. We just have way too many people with no ldap and they should be able to run this as well. Do we have to modify the .drone.yml as well?
I tried several different ways on how to run the openldap docker image, but none worked. I always get "couldn't connect to authentication service". I'm on windows and used the command you provided in the Readme. I adapted the path to my local one and used both host and bridge network and in the configuration I used the IP address, localhost, openldap and none worked. I'm not a pro with Docker, so maybe you can help me?
And just when I wrote that above, I got it to work. With the command given in the Readme, you get a local server which you can access with localhost:[port]. @tatankat, if you can mark the tests as skipped if ldap_host is empty, I would mark this as a successfull test.
@Hackwar Thanks for giving the ldap server a try! I implemented your proposed change and made the documentation a bit more clear.
@Hackwar do you expect something else from me?
Once this is merged, I will be able to:
- enable the test for direct bind for which the fix has been merged
- enable the test in #37962
- fix a problem reported by phan (which points rightfully to a bug) and add tests for it to check attribute values. As these changes will probably introduce additional conflicts, I am stuck waiting for this to be merged.
I don't know how to fix the existing conflict here. The changes in this PR can overwrite the typo fixes done in #39357 as the documentation is completely updated.
