Add Port To WP_TESTS_DOMAIN#49883
Add Port To WP_TESTS_DOMAIN#49883noahtallen merged 9 commits intoWordPress:trunkfrom ObliviousHarmony:fix/wp-env-tests-domain
WP_TESTS_DOMAIN#49883Conversation
This new function will allow us to add/replace ports in certain `wp-config.php` values.
Since `WP_SITEURL` already contains the port in both the development and test environments, there is no reason to try and construct the URL with the port. Additionally, it is more accurate to use the test environment's `WP_SITEURL` since that's what WordPress will expect due to the constant.
Tests should use the `WP_TESTS_DOMAIN` constant to construct URLs in tests. This poses a problem, however, because the port will be part of the URL in test environments. In my review of WordPress Core and WooCommerce, adding the port to `WP_TESTS_DOMAIN` should not break anything and will make the container more resilient to port customizations.
To add resiliency to the test suite, we should use `WP_TESTS_DOMAIN` instead of hardcoding the domain and port in the URLs.
| "TEST_VAL3": false, | ||
| "WP_DEBUG": true, | ||
| "WP_ENVIRONMENT_TYPE": "local", | ||
| "WP_HOME": "http://localhost:2000/", |
There was a problem hiding this comment.
As far as I can tell, we aren't supposed to put trailing slashes on these constants. They're actually removed automatically by WordPress.. Since my new port addition function doesn't add a trailing slash, we don't have one here, but that's okay.
| * @param {Object} spinner A CLI spinner which indicates progress. | ||
| */ | ||
| async function configureWordPress( environment, config, spinner ) { | ||
| const url = ( () => { |
There was a problem hiding this comment.
Is it just me or was this previously setting http://localhost:8888:8888 for the development instance? Technically the port is already on WP_SITEURL here because of updateEnvUrl right? For what it's worth, in my testing of WP_TESTS_DOMAIN with a port (without this PR), it seems like WordPress ignores the second port entirely. Resiliency!
|
I think this might conflict with a previous PR here: #49082:
That suggests we shouldn't add a port to wp_tests_domain 🤔 |
|
Looking at the PR that I think you meant to link to, of note is the example construction that they gave: The problem was that URLs given by the site ended up being |
noahtallen
added
[Type] Enhancement
| public function test_render_block_core_file() { | ||
| $attributes = array( | ||
| 'href' => 'http://localhost:8889/wp-content/uploads/2021/04/yolo.pdf', | ||
| 'href' => 'http://' . WP_TESTS_DOMAIN . '/wp-content/uploads/2021/04/yolo.pdf', |
noahtallen
left a comment
noahtallen
left a comment
There was a problem hiding this comment.
Nice, I'm very happy that this makes our URL handling much more resilient and consistent.
bph
added
[Type] Bug
|
I think this PR broke all wp-env instances that actually use port 80. The fronend wants 'localhost:80' as URL, but the browser removes port 80 by default, leading to a redirect loop. |
|
Hi @ObliviousHarmony, I had also just worked in parallel on an improvement for wp-env. My improvement prevents the "port is already allocated" issue. It's about port handling: #49843 . I will now try to adapt my changes in your massive changes and hope it works :-) FYI: I wrote a script that outputs a secure (unused) port number. |
5 participants
What?
This PR adds the configured port to
WP_TESTS_DOMAINso that tests referencing'http://' . WP_TESTS_DOMAIN . /pathwill work with a port other than 80.Why?
I am currently working on running unit tests within the container, however, I have run into problems because the URLs generated by WordPress contain the port while
WP_TESTS_DOMAINdoes not. By adding the port to the constant we are able to make tests more resilient and better support environment customization.I have looked at WordPress' tests and there doesn't seem to be anything wrong with just adding the port to the constant. This also removes the need for the port in Gutenberg's tests, which this PR removes.
How?
This pull request refactors the way that ports are added to configuration options. This let me write some nice tests for different cases and handle inputs that aren't URLs. To keep the validation in place I have added the URL validation to the config validation function instead.
In the process of making these changes I also noticed that the
--urlbeing passed to WordPress' installation command has the port included twice. There's no reason to use anything other thanWP_SITEURLsince the constant will override it anyway. It will have the correct port and is the most accurate input to use here.Testing Instructions
.wp-env.override.jsonfile that changes the port of the test container.npm run env startworks with different ports and that the containers work as you'd expect them to (correct ports in URLs, etc).