Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#51670 closed defect (bug) (fixed)

Multisite test failures on GitHub Actions

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: desrosj's profile desrosj
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

Background: #50401, #49594.

Starting with [49369], there are now some failures in Multisite tests on GitHub Actions, as seen in this build for example:

1) Tests_L10n::test_wp_get_installed_translations_for_core
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'2016-10-26 00:01+0200'
+'2020-08-28 07:36:49+0000'

/var/www/tests/phpunit/tests/l10n.php:79

2) Tests_Locale_Switcher::test_switch_to_site_locale_if_user_locale_is_set
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'de_DE'
+'de'

/var/www/tests/phpunit/tests/l10n/localeSwitcher.php:298

3) Tests_Locale_Switcher::test_switch_to_different_site_locale_if_user_locale_is_set
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'de_DE'
+'de'

/var/www/tests/phpunit/tests/l10n/localeSwitcher.php:351

4) Tests_Privacy_WpPrivacySendPersonalDataExportEmail::test_should_send_personal_data_export_email_in_user_locale
Failed asserting that '[Test Blog] Personal Data Export' contains "Exportación de datos personales".

/var/www/tests/phpunit/tests/privacy/wpPrivacySendPersonalDataExportEmail.php:344

5) Tests_Privacy_WpPrivacySendPersonalDataExportEmail::test_should_send_personal_data_export_email_in_user_locale_when_site_is_not_en_us
Failed asserting that '[Test Blog] Personal Data Export' contains "Export personenbezogener Daten".

/var/www/tests/phpunit/tests/privacy/wpPrivacySendPersonalDataExportEmail.php:366

6) Tests_Privacy_WpPrivacySendPersonalDataExportEmail::test_should_send_personal_data_export_email_in_user_locale_when_admin_and_site_have_different_locales
Failed asserting that '[Test Blog] Personal Data Export' contains "Exportación de datos personales".

/var/www/tests/phpunit/tests/privacy/wpPrivacySendPersonalDataExportEmail.php:388

7) Tests_Privacy_WpPrivacySendPersonalDataExportEmail::test_should_send_personal_data_export_email_in_user_locale_when_both_have_different_locales_than_site
Failed asserting that '[Test Blog] Personal Data Export' contains "Export personenbezogener Daten".

/var/www/tests/phpunit/tests/privacy/wpPrivacySendPersonalDataExportEmail.php:412

8) Tests_Privacy_WpPrivacySendPersonalDataExportEmail::test_should_send_personal_data_export_email_in_site_locale_when_not_en_us_and_admin_has_different_locale
Failed asserting that '[Test Blog] Personal Data Export' contains "Exportación de datos personales".

/var/www/tests/phpunit/tests/privacy/wpPrivacySendPersonalDataExportEmail.php:459

These tests seem to pass as expected on Travis, so this might be something specific to GitHub Actions.

After some initial debugging, it looks like for some reason these tests end up using the latest translation files from GlotPress, instead of the ones from the tests/phpunit/data/languages directory, even though the tests bootstap file specifically sets WP_LANG_DIR to DIR_TESTDATA . '/languages'. This will need more investigation.

Attachments (1)

51670.diff (762 bytes) - added by desrosj 4 years ago.

Download all attachments as: .zip

Change History (9)

#1 @desrosj
4 years ago

  • Keywords needs-testing added

I'm taking a look at this. It seems that reverting [49369] does not fix the issue, so it's interesting that it started at that point, as nothing was touched related to language tests.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


4 years ago

This ticket was mentioned in PR #683 on WordPress/wordpress-develop by ocean90.


4 years ago
#3

  • Keywords has-patch has-unit-tests added

The tests for WP_REST_Plugins_Controller::create_item() in WP_REST_Plugins_Controller_Test are doing real api.w.org requests which will also return data about outdated translations. If the tests are run again, Language_Pack_Upgrader::sync_upgrade() will now update the translations based on the previous data.

Trac ticket: https://core.trac.wordpress.org/ticket/51670

desrosj commented on PR #683:


4 years ago
#4

This looks good to me, and I also think I know what caused this to start happening all of a sudden.

In the commit in question, I also expanded the list of excluded files when packaging the ZIP file during the setup job to include .git*, and packagehash.txt. If packagehash.txt was present on the actual test jobs, it would prevent the npx install-changed command from running the install command. I added the .git* exclusion to prevent the .git folder from also being included.

The code right before apply_filters( 'async_update_translation' ) runs the is_vcs_checkout() check and returns early if the site appears to be under version control. When the .git folder was excluded from the ZIP file, the follow up jobs in the workflow that run the tests would no longer assume that the install is under version control, and the language packs would be updated.

It's not clear to me whether running the tests with the assumption that the install is under version control is preferred, though. If it is, we should probably override the is_vcs_checkout() function with the automatic_updates_is_vcs_checkout filter in the test suite itself to always guarantee the site will be viewed as under VCS.

TimothyBJacobs commented on PR #683:


4 years ago
#5

I thought it was blocked because that is only hooked in the admin? https://core.trac.wordpress.org/ticket/50732#comment:17

@desrosj
4 years ago

#6 @desrosj
4 years ago

  • Keywords commit added; needs-testing removed

Props to @ocean90 for working through this with me (and doing most of the work).

51670.diff is the solution we arrived at.

Instead of forcing the version control check to be false or only disabling async_update_translation only in the class causing the issue, 51670.diff disables auto-update and async translation updates attempts for the entire test suite.

There are no PHPUnit tests for this code, and they are more appropriate for E2E tests anyway. If a unit tests needs to test these two things, they can be re-enabled with another add_filter call inside the relevant test function.

#7 @desrosj
4 years ago

  • Owner set to desrosj
  • Resolution set to fixed
  • Status changed from new to closed

In 49491:

Build/Test Tools: Disable update attempts while running unit tests.

This fixes an issue introduced in [49369] that causes l10n related tests to fail when the PHPUnit test suite is run multiple times without hints of the site being under version control.

[49369] removed the .git folder from the ZIP artifact created during the initial setup job. This ZIP file is used by the later jobs in the workflow that run the test suite. The absence of the .git folder in these later jobs caused the language packs initially loaded from phpunit/data/languages folder to be updated asynchronously, resulting in unexpected values when running the tests a second time.

This change disables all Core auto-update and asynchronous language pack update attempts when running PHPUnit tests.

Props ocean90, SergeyBiryukov.
See #50401.
Fixes #51670.

This ticket was mentioned in Slack in #core by sergey. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.