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

Block fixtures: Change port to 8889 to placate KSES #37100

Merged
merged 1 commit into from
Dec 3, 2021

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Dec 3, 2021

Description

Unit tests are currently failing on trunk (when used with a current snapshot of the WordPress svn or git repo).

Specifically: Block_Fixture_Test::test_kses_doesnt_change_fixtures fails for datasets #54 and #55.

Reading WordPress/wordpress-develop#1998, I believe that the reason might be that our KSES routines are verifying that the specified port matches the one of the local install. And since our tests normally run localhost:8889, I'm updating the relevant test fixtures (from previously 8888) to reflect that.

How has this been tested?

Verify that CI goes green 🤞

Types of changes

Bug fix

@ockham ockham added [Type] Bug An existing feature does not function as intended [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Dec 3, 2021
@ockham ockham self-assigned this Dec 3, 2021
@gziolo
Copy link
Member

gziolo commented Dec 3, 2021

The unit tests runner handles those integrations tests and it has the following URL configured:

testURL: 'http://localhost',

@ockham
Copy link
Contributor Author

ockham commented Dec 3, 2021

Thanks @gziolo. The PR does seem to fix the fixture errors tho (although there's an unrelated error that's still present) -- compare e.g. https://github.com/WordPress/gutenberg/runs/4408490770?check_suite_focus=true (current trunk) to https://github.com/WordPress/gutenberg/runs/4408762944?check_suite_focus=true (this PR).

Curious to hear your suggestion -- are you saying you'd rather remove the port altogether from the fixtures? Or is it okay to go ahead with this fix as-is?

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

You applied the changes correctly 👍 The same test fixtures are also used with PHPUnit, so it'd be the reason why it needs the same port. It probably is less critical for unit tests, but maybe we should align that separately from this PR.

I got confused because of the location of fixtures 🙈

@ockham
Copy link
Contributor Author

ockham commented Dec 3, 2021

Thanks @gziolo -- I'll go ahead and merge 😄


FWIW, the remaining PHP unit test that's failing

1) WP_REST_URL_Details_Controller_Test::test_will_return_from_cache_if_populated
Failed asserting that 'Example Website — - with encoded content.' contains "This value from cache".

/var/www/html/wp-content/plugins/gutenberg/phpunit/class-wp-rest-url-details-controller-test.php:302

is probably due to WordPress/wordpress-develop@1270c89 (committed today at 11:34 UTC): I noticed it wasn't present on trunk for https://github.com/WordPress/gutenberg/runs/4407241926?check_suite_focus=true (at 11:15 UTC) but for the following merge https://github.com/WordPress/gutenberg/runs/4408138725?check_suite_focus=true (at 12:51 UTC), and it touches src/wp-includes/rest-api/endpoints/class-wp-rest-url-details-controller.php which seems relevant to the error 😬

cc/ @audrasjb

@ockham ockham merged commit d7d2484 into trunk Dec 3, 2021
@ockham ockham deleted the fix/unit-tests-port-number branch December 3, 2021 15:35
@github-actions github-actions bot added this to the Gutenberg 12.2 milestone Dec 3, 2021
@ramonjd
Copy link
Member

ramonjd commented Dec 3, 2021

Thanks for looking at this @ockham !

@ockham
Copy link
Contributor Author

ockham commented Dec 3, 2021

FWIW, the remaining PHP unit test that's failing

1) WP_REST_URL_Details_Controller_Test::test_will_return_from_cache_if_populated
Failed asserting that 'Example Website — - with encoded content.' contains "This value from cache".

/var/www/html/wp-content/plugins/gutenberg/phpunit/class-wp-rest-url-details-controller-test.php:302

[...]

Filed an issue for this: #37116

noisysocks pushed a commit that referenced this pull request Dec 6, 2021
Unit tests are currently failing on `trunk` (when used with a current snapshot of the WordPress svn or git repo).

Specifically: `Block_Fixture_Test::test_kses_doesnt_change_fixtures` fails for datasets #54 and #55.

Reading WordPress/wordpress-develop#1998, I believe that the reason might be that our KSES routines are verifying that the specified port matches the one of the local install. And since our tests normally run `localhost:8889`, I'm updating the relevant test fixtures (from previously `8888`) to reflect that.
youknowriad pushed a commit that referenced this pull request Dec 8, 2021
Unit tests are currently failing on `trunk` (when used with a current snapshot of the WordPress svn or git repo).

Specifically: `Block_Fixture_Test::test_kses_doesnt_change_fixtures` fails for datasets #54 and #55.

Reading WordPress/wordpress-develop#1998, I believe that the reason might be that our KSES routines are verifying that the specified port matches the one of the local install. And since our tests normally run `localhost:8889`, I'm updating the relevant test fixtures (from previously `8888`) to reflect that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants