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

Prevent DB warnings in unit tests #339

Merged
merged 5 commits into from
Dec 18, 2023

Conversation

mikkamp
Copy link
Contributor

@mikkamp mikkamp commented Dec 15, 2023

Changes proposed in this Pull Request:

This PR reverts the change in #335 since it still produces warnings when starting tests with an empty database. So here we revert to an earlier hook and add a filter to prevent the notice from showing. We also use WC_REMOVE_ALL_DATA so all database tables can be removed and recreated when WC_Install is called.

The PR also adds PHP version 8.3 to the workflow, so it now runs the default tests on PHP 8.2 and includes the oldest supported version (7.4) and the latest supported version (8.3).

Closes #338

Detailed test instructions:

  1. Install unit tests with a version of WC > 8.3 bin/install-unit-tests.sh <db_name> <db_user> <db_pass> (select Y to clear database during install)
  2. Run unit tests vendor/bin/phpunit
  3. Confirm there are no longer any warnings when the tests are run
  4. Check the Workflow tests and confirm we have successful runs for PHP 7.4, 8.2, 8.3

Changelog entry

  • Dev - Prevent DB warnings in unit tests.

@mikkamp mikkamp self-assigned this Dec 15, 2023
@github-actions github-actions bot added the changelog: dev Developer-facing only change. label Dec 15, 2023
@mikkamp mikkamp marked this pull request as ready for review December 15, 2023 14:36
@mikkamp mikkamp requested a review from a team December 15, 2023 14:36
Copy link
Contributor

@puntope puntope left a comment

Choose a reason for hiding this comment

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

Thanks @mikkamp I don't see anymore the errors in the console and the tests pass. Also now the file seems consistent with the AW one.

❓ In woocommerce/google-listings-and-ads#2183 you mentioned that we should mark extensions as compatible with PHP 8.3, Should we mark this one as Composer as well as the tests run smoothly? Or it's any other step to take?

@mikkamp
Copy link
Contributor Author

mikkamp commented Dec 18, 2023

❓In woocommerce/google-listings-and-ads#2183 you mentioned that we should mark extensions as compatible with PHP 8.3, Should we mark this one as Composer as well as the tests run smoothly? Or it's any other step to take?

I've been marking them as compatible in the project thread: peeuvX-19l-p2
I'm not sure I follow about marking it as compatible in composer, I don't recall us generally setting any max PHP version, at most we have extensions which set a minimum PHP version: https://github.com/woocommerce/google-listings-and-ads/blob/2.5.13/composer.json#L8
But that's only for extensions that include composer packages in the release.

@mikkamp mikkamp merged commit 681ff54 into trunk Dec 18, 2023
7 checks passed
@mikkamp mikkamp deleted the dev/338-fix-db-warnings-in-unit-tests branch December 18, 2023 08:51
@puntope
Copy link
Contributor

puntope commented Dec 18, 2023

❓In woocommerce/google-listings-and-ads#2183 you mentioned that we should mark extensions as compatible with PHP 8.3, Should we mark this one as Composer as well as the tests run smoothly? Or it's any other step to take?

I've been marking them as compatible in the project thread: peeuvX-19l-p2 I'm not sure I follow about marking it as compatible in composer, I don't recall us generally setting any max PHP version, at most we have extensions which set a minimum PHP version: https://github.com/woocommerce/google-listings-and-ads/blob/2.5.13/composer.json#L8 But that's only for extensions that include composer packages in the release.

What I meant about composer is that Composer install runs successfully. As far as I read that's a condition to make it compatible.

@puntope puntope mentioned this pull request Dec 19, 2023
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: dev Developer-facing only change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unit testing outputs warnings when starting with a clean database
2 participants