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

Use GitHub actions for Continuous Integration tests #89

Merged
merged 3 commits into from
Nov 28, 2022

Conversation

thePanz
Copy link
Member

@thePanz thePanz commented Nov 24, 2022

No description provided.

@thePanz thePanz force-pushed the switch-to-github-actions branch 5 times, most recently from 8744814 to ef6d1df Compare November 24, 2022 17:16
@thePanz thePanz self-assigned this Nov 24, 2022
@thePanz thePanz force-pushed the switch-to-github-actions branch from 825ac32 to 9eb0aa2 Compare November 24, 2022 22:01
@thePanz
Copy link
Member Author

thePanz commented Nov 24, 2022

Poke @alquerci @Tybaze and @xNatek as you are the ones that mainly worked on the PHP v8.x compatibility

Notes:

  • apparently a running MySQL service is not needed to complete the tests
  • the tests are failing if run in a different order: the list of open connections is shared among all tests, leading to a bad isolation → will not address it here :)

@alquerci
Copy link

alquerci commented Nov 27, 2022

@thePanz I do not know GitHub actions environment, is it possible to execute tests//bin/test?

GitHub actions environment is not controlled. We should not depend on an external service to dictate the test environment.
More over, having a different environment during development and on CI is not good.

The CI is a detail, we should be able to change it without changing the test code.

apparently a running MySQL service is not needed to complete the tests

After removing db service on docker-compose, tests failed.
So it seems that GitHub actions environment provide a mysql server.

db connection bad isolation

Indeed.

@thePanz
Copy link
Member Author

thePanz commented Nov 28, 2022

@alquerci feels weird that the GitHub Actions has a mysql service up and running.

All the current action is doing is cd tests && php run.php, this is the same that is being done in tests//bin/test

In the meantime, do you check by runnning the current HEAD code from this repository that the changes are still working ok with your docker setup?

@alquerci
Copy link

do you check by runnning the current HEAD code from this repository that the changes are still working ok with your docker setup?

Yes, that exactly what I do when I tested your hypothesis.

All green.

@Tybaze
Copy link
Collaborator

Tybaze commented Nov 28, 2022

The Ubuntu22.04 image contains Mysql by default, but it need to be started
https://github.com/actions/runner-images/blob/ubuntu22/20221119.2/images/linux/Ubuntu2204-Readme.md

Here the image install script which should disabled it in systemd
https://github.com/actions/runner-images/blob/main/images/linux/scripts/installers/mysql.sh

I have tested the command on a fresh Ubuntu 22.04 and it stop and disable mysql,
And I'm unable to find into doctrine1 actions test scripts where the systemctl start mysql.service is executed

I confirm, that some Doctrine1 Test, require mysql with : user:root password:password
A random example :
Doctrine_Ticket_1830_TestCase

But Mysql on Ubuntu22.04 use the plugin "auth_socket" which allow any connection from the socket, so without configuring any credentials, so Doctrine test with root:password works

Ubuntu images configure Mysql to root:root but it's pretty uselesss without changing the "auth_socket".


I have run the test on docker, all green too !

@thePanz
Copy link
Member Author

thePanz commented Nov 28, 2022

Thanks @Tybaze , maybe systemd is starting the service as soon the socket is used. I recall reading something like that for another service, but can't remember where nor any link to it.

So,

  • the good part: yes, the tests are covering MySQL
  • the bad: the tests are not/badly isolated and the ConnectionManager singleton is keeping the list of all connections, and tests are relying and using the "latest" (or one of the many) connection available there.

@thePanz thePanz merged commit 92abb24 into master Nov 28, 2022
@thePanz thePanz deleted the switch-to-github-actions branch November 28, 2022 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants