WordPress.org

Make WordPress Core

Opened 6 weeks ago

Last modified 4 weeks ago

#50251 reopened enhancement

Report if populate_network() fails during test installation

Reported by: TimothyBlynJacobs Owned by: jorbin
Milestone: 5.5 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

While looking into a Gutenberg phpunit issue we discovered that the root cause of tests failing with <h1>Error establishing a database connection</h1> was that populate_network() was failing because of a missing WP_TESTS_EMAIL.

I think it'd be helpful if we reported to the user if populate_network() returned a WP_Error instance

Change History (8)

This ticket was mentioned in PR #309 on WordPress/wordpress-develop by TimothyBJacobs.


5 weeks ago

  • Keywords has-patch has-unit-tests added

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


5 weeks ago

#3 @SergeyBiryukov
5 weeks ago

  • Milestone changed from Awaiting Review to 5.5

#4 @jorbin
5 weeks ago

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

In 47904:

Build/Test: Die with an error if populate_network fails

If you are missing WP_TESTS_EMAIL, populate_network will fail and it can be hard to debug. As populate_network can return a wp_error object, we can detect that and display the error to a user.

See: https://github.com/WordPress/gutenberg/pull/22613
Fixes: #50251
Props: TimothyBlynJacobs

#5 @azaozz
5 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[47904] would prevent the tests from running when WP_TESTS_DOMAIN or WP_TESTS_EMAIL are not defined, however this points to bigger/other problems. WP_TESTS_DOMAIN and WP_TESTS_EMAIL are defined in https://core.trac.wordpress.org/browser/tags/5.4.1/wp-tests-config-sample.php#L68 and should always be present.

As far as I see the actual problem here is that wp-tests-config.php is not present (loaded), or that not all of the constants (settings) there are replaced when that file is not used. Perhaps it would be better to validate that all needed constants are present while doing the bootstrap instead of attempting to run and failing.

This gets a bit trickier as phpunit/includes/install.php is run in another PHP process from cli, see https://core.trac.wordpress.org/browser/tags/5.4.1/tests/phpunit/includes/bootstrap.php#L100.

Last edited 5 weeks ago by azaozz (previous) (diff)

#6 follow-up: @TimothyBlynJacobs
5 weeks ago

that not all of the constants (settings) there are replaced when that file is not used.

Our issue was that not all of the constants were loaded, and tracking this down was incredibly difficult because the eventual error message that gets generated is seemingly unrelated.

Perhaps it would be better to validate that all needed constants are present while doing the bootstrap instead of attempting to run and failing.

I think checking that all the constants are defined as expected would be great. I think the populate_network check is still valuable as well because the checks there are more intricate than just checking for a non-empty value.

#7 in reply to: ↑ 6 @azaozz
5 weeks ago

Replying to TimothyBlynJacobs:

I think the populate_network check is still valuable as well

Yes, definitely. Just thinking it would be good to get to the "absolute bottom" of this and fix it once and for all :)

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


4 weeks ago

Note: See TracTickets for help on using tickets.