Skip to content

Conversation

jkmassel
Copy link
Contributor

@jkmassel jkmassel commented May 22, 2024

This PR fixes a flaky test case for WordPress 5.7 – it can't connect to a MySQL 8+ instance (most of the time, weirdly – it sometimes works in CI?!). The error messaging wasn't clear though – it really looked like a race condition between MySQL being ready and the script running (because the error just said "unable to connect to database").

This PR:

  • Switches the test DB to MariaDB instead of MySQL (though I kept the mysql container name for backward compatibility)
  • Removes the docker-based dependency management, and moves responsibility for waiting for the DB to be present to setup-test-site.sh. This allows the docker container to start its work right away, cutting about 20 seconds off the setup time while improving reliability by opening a connection from the wordpress container to the mysql one before attempting any further setup. wp db check uses mysqlcheck under the hood, so we get detailed error information if the connection fails.
  • Removes an (AFAICT unneeded) mysql configuration file
  • Creates the wp-cli cache directory to silence a warning

To test:

  • Ensure that CI passes

@jkmassel jkmassel force-pushed the fix/flaky-network-tests branch 11 times, most recently from ee736c9 to 623368f Compare May 22, 2024 20:49
@jkmassel jkmassel requested review from crazytonyli and oguzkocer May 22, 2024 20:50
@jkmassel jkmassel marked this pull request as ready for review May 22, 2024 20:50
@jkmassel jkmassel enabled auto-merge (squash) May 22, 2024 20:50
@jkmassel jkmassel force-pushed the fix/flaky-network-tests branch from 623368f to be3ca31 Compare May 22, 2024 21:16
@jkmassel jkmassel force-pushed the fix/flaky-network-tests branch from be3ca31 to d289731 Compare May 22, 2024 21:26
@jkmassel jkmassel self-assigned this May 22, 2024
@jkmassel jkmassel added the Bug label May 22, 2024
@jkmassel jkmassel added this to the 0.1 milestone May 22, 2024
Copy link
Contributor

@oguzkocer oguzkocer left a comment

Choose a reason for hiding this comment

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

Switches the test DB to MariaDB instead of MySQL (though I kept the mysql container name for backward compatibility)

Could you elaborate on backward compatibility bit? Not sure what it'll break, but I think we should just change the name.

Removes an (AFAICT unneeded) mysql configuration file

This was used by dump-mysql & restore-mysql Make tasks, which this PR breakes.


The PR is breaking the Rust integration tests - something crucial for my work. Can you please ensure that cargo test --test '*' -- --test-threads 1 passes for your changes? Probably worth updating the testing instructions for the PR as well.

Until this change, I was using the following commands to run the integration tests. These won't work right now, but I thought it might be useful to you:

To Test

  • make test-server && make dump-mysql && make backup-wp-content-plugins
  • cargo test --test '*' -- --nocapture --test-threads 1

@jkmassel
Copy link
Contributor Author

@oguzkocer I've fixed compatibility (including renaming the container) in 6bf72d6

It also gets rid of the need for the extra mysql config file – our setup has zero security as it is, so it's fine to pass the details on the command line

@jkmassel jkmassel requested a review from oguzkocer May 22, 2024 22:21
@jkmassel jkmassel merged commit 1cf8d71 into trunk May 22, 2024
@jkmassel jkmassel deleted the fix/flaky-network-tests branch May 22, 2024 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants