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

Fix MySQL and MSSQL test failures #16011

Closed
wants to merge 3 commits into from

Conversation

KentarouTakeda
Copy link
Contributor

@KentarouTakeda KentarouTakeda commented Sep 23, 2024

This is a related fix to #15997.

  • MySQL: Many tests were randomly failing because they were also trying to start locally, even though they were using a service container. I removed those unnecessary local starts.
    • UPDATE: 2024-09-24 01:25 JST Since the nightly test was run locally, I unified it to use the service container.
  • MSSQL: The container image used for MSSQL tests was very old. It appears that this broke upstream recently. I updated to a more recent container image and fixed the setup command accordingly.

@Ayesh
Copy link
Member

Ayesh commented Sep 23, 2024

But we are not running those mysql tests then? Shouldn't we be using mysql in a container at least then?

@KentarouTakeda
Copy link
Contributor Author

No problem. It's running as a container here:

mysql:
image: mysql:8.3
ports:
- 3306:3306
env:
MYSQL_DATABASE: test
MYSQL_ROOT_PASSWORD: root

(But it was still running locally)

There are some parts that are run locally without using containers. This is for benchmarking purposes.
(Probably because we don't want communication with containers to become a bottleneck)

Of course, they are left in place:

sudo service mysql start
mysql -uroot -proot -e "CREATE DATABASE IF NOT EXISTS wordpress"
mysql -uroot -proot -e "CREATE USER 'wordpress'@'localhost' IDENTIFIED BY 'wordpress'; FLUSH PRIVILEGES;"
mysql -uroot -proot -e "GRANT ALL PRIVILEGES ON *.* TO 'wordpress'@'localhost' WITH GRANT OPTION;"

Just to be sure, I'll check the test results with artifacts later.

@Ayesh
Copy link
Member

Ayesh commented Sep 23, 2024

Right, thank you. This makes sense. I misread the PR, sorry about that.

@KentarouTakeda KentarouTakeda mentioned this pull request Sep 23, 2024
@cmb69
Copy link
Member

cmb69 commented Sep 23, 2024

Now, look at this: "All checks have passed" Thank you, @KentarouTakeda!

cc @iluuu1994

@iluuu1994
Copy link
Member

Thank you! Indeed, we seem to have switched to mysql containers in 8.3, I forgot about that. Can you target 8.3 then?

@cmb69
Copy link
Member

cmb69 commented Sep 23, 2024

Watch out! It seems to me that nightly doesn't use a mysql container, what might be a problem.

Also we need to port this to PHP-8.2, and possibly PHP-8.1.

@KentarouTakeda KentarouTakeda changed the base branch from master to PHP-8.3 September 23, 2024 15:53
@KentarouTakeda
Copy link
Contributor Author

The nightly tests have also been unified to start the service container.
(Updated the pull request description. Thanks to @cmb69!)

The pull request target has been changed to 8.3. It may take some time because I wasn't sure how to deal with conflicts in 8.2 and 8.1, and it's already late at night in Japan and I have work tomorrow. I'm sorry.

If we need to hurry, please feel free to use my pull request as needed.

@iluuu1994
Copy link
Member

@cmb69 8.2 doesn't use a mysql container yet, so 8.3 should be enough.

@cmb69
Copy link
Member

cmb69 commented Sep 23, 2024

8.2 doesn't use a mysql container yet, so 8.3 should be enough.

Ah, right, PHP-8.2 wasn't affected by this issue at all.

Copy link
Member

@SakiTakamachi SakiTakamachi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

- 3306:3306
env:
MYSQL_DATABASE: test
MYSQL_ROOT_PASSWORD: root
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is problematic, because it will add the service to older branches in nightly, at which point the sudo service mysql start again needs to be removed. So maybe this needs to target 8.2 after all. Or we can skip the service somehow on older branches, but I don't think that's possible, I don't see such an option in the documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, just committed to PHP-8.3 and master (I want todays tags to run on all CI). What to do now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmb69 No worries. It was broken anyway. I think it would be easiest if we backport this and consistently use containers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmb69 Are you still on Slack? If so, maybe it would be easier to discuss it there. 🙂 If not, I can organize a new invite for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iluuu1994 @cmb69
It seems that I confused you because I didn't understand how backporting and Nightly Test work. Thank you very much for all your consideration and support.

Is it okay for me to ask everyone to backport? Please let me know if there's anything I can do for you.

@cmb69 cmb69 closed this in af721c9 Sep 24, 2024
@KentarouTakeda KentarouTakeda deleted the fix-mysql-test branch September 24, 2024 13:51
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.

5 participants