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

Facilitate running tests #38775

Merged
merged 10 commits into from
Jan 26, 2023
Merged

Facilitate running tests #38775

merged 10 commits into from
Jan 26, 2023

Conversation

tatankat
Copy link
Contributor

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

@tatankat
Copy link
Contributor Author

tatankat commented Sep 20, 2022

@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.

@Hackwar
Copy link
Member

Hackwar commented Nov 8, 2022

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?

@Hackwar
Copy link
Member

Hackwar commented Nov 11, 2022

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?

@Hackwar
Copy link
Member

Hackwar commented Nov 11, 2022

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.

@tatankat
Copy link
Contributor Author

@Hackwar Thanks for giving the ldap server a try! I implemented your proposed change and made the documentation a bit more clear.

@tatankat
Copy link
Contributor Author

@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 [4.x] Fix LDAP over SSL #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.

@laoneo
Copy link
Member

laoneo commented Jan 23, 2023

Can you fix the conflicts?

@laoneo
Copy link
Member

laoneo commented Jan 23, 2023

The integration tests are failing.

@tatankat
Copy link
Contributor Author

No, they aren't ;)

@laoneo
Copy link
Member

laoneo commented Jan 23, 2023

We will see...

@laoneo
Copy link
Member

laoneo commented Jan 26, 2023

Can you fix the conflict here?

@laoneo laoneo merged commit c720afa into joomla:4.3-dev Jan 26, 2023
@laoneo
Copy link
Member

laoneo commented Jan 26, 2023

Thank you very much! Will adapt now my unit test pr.

@laoneo laoneo added this to the Joomla! 4.3.0 milestone Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants