Make WordPress Core

Opened 4 weeks ago

Last modified 7 days ago

#61530 new task (blessed)

Test tool and unit test improvements for 6.7

Reported by: jonsurrell's profile jonsurrell Owned by:
Milestone: 6.7 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch has-unit-tests
Focuses: Cc:

Description (last modified by jorbin)

This ticket is for various fixes and improvements in PHPUnit tests that don't have a more specific ticket, as well as general improvements to the GitHub Actions workflows that run automated testing.

Change History (26)

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


4 weeks ago

This ticket was mentioned in PR #6765 on WordPress/wordpress-develop by @jonsurrell.


4 weeks ago
#2

  • Keywords has-patch has-unit-tests added

Add tests for comment handling.

These tests caught a regression during the 6.6 development cycle (with processing instruction comment tag name) that was fixed, but the tests were not merged.

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

#3 @jorbin
4 weeks ago

  • Component changed from HTML API to Build/Test Tools
  • Description modified (diff)
  • Summary changed from HTML API: Add more tests targeting 6.7 release to Test tool and unit test improvements for 6.7
  • Type changed from enhancement to task (blessed)

@jonsurrell commented on PR #6765:


4 weeks ago
#4

Good suggestions, thanks @aaronjorbin. I've applied them.

This ticket was mentioned in PR #6765 on WordPress/wordpress-develop by @jonsurrell.


4 weeks ago
#5

Trac ticket: Core-61530

Add tests for comment handling.

These tests caught a regression during the 6.6 development cycle (with processing instruction comment tag name) that was fixed, but the tests were not merged.

#6 @SergeyBiryukov
4 weeks ago

In 58594:

Tests: Use assertSame() in WP_Interactivity_API tests.

This ensures that not only the return values match the expected results, but also that their type is the same.

Going forward, stricter type checking by using assertSame() should generally be preferred to assertEquals() where appropriate, to make the tests more reliable.

Follow-up to [57563], [57649], [57822], [57826], [57835], [58159], [58327].

See #61530.

This ticket was mentioned in PR #6765 on WordPress/wordpress-develop by @jonsurrell.


8 days ago
#7

Trac ticket: Core-61530

Add tests for comment handling.

These tests caught a regression during the 6.6 development cycle (with processing instruction comment tag name) that was fixed, but the tests were not merged.

@jonsurrell commented on PR #6765:


8 days ago
#8

Thanks @dmsnell, I applied those suggestions and cleaned things up. The signatures do become much simpler when we split the #comment and #funky-comment tests.

This ticket was mentioned in PR #6765 on WordPress/wordpress-develop by @jonsurrell.


8 days ago
#9

Trac ticket: Core-61530

Add tests for comment handling.

These tests caught a regression during the 6.6 development cycle (with processing instruction comment tag name) that was fixed, but the tests were not merged.

This ticket was mentioned in PR #6765 on WordPress/wordpress-develop by @jonsurrell.


8 days ago
#10

Trac ticket: Core-61530

Add tests for comment handling.

These tests caught a regression during the 6.6 development cycle (with processing instruction comment tag name) that was fixed, but the tests were not merged.

This ticket was mentioned in PR #6765 on WordPress/wordpress-develop by @jonsurrell.


8 days ago
#11

Trac ticket: Core-61530

Add tests for comment handling.

These tests caught a regression during the 6.6 development cycle (with processing instruction comment tag name) that was fixed, but the tests were not merged.

This ticket was mentioned in PR #7039 on WordPress/wordpress-develop by @jrf.


7 days ago
#12

The ${ syntax for string interpolation was deprecated in PHP 8.2 and should not be used anymore.

Ref: https://wiki.php.net/rfc/deprecate_dollar_brace_string_interpolation

Trac ticket: https://core.trac.wordpress.org/ticket/61530
Trac ticket: https://core.trac.wordpress.org/ticket/59654

#13 @jrf
7 days ago

PR GH 7039 fixes a newly introduced PHP 8.2 deprecation notice.

This was not caught by running the tests as this is a deprecation notice which is thrown at compile time, not runtime.

#14 @hellofromTonya
7 days ago

Patch: https://github.com/WordPress/wordpress-develop/pull/7039

Before this patch: the deprecation is thrown at the start of running the PHP 8.2+ tests ([see it here](https://github.com/WordPress/wordpress-develop/actions/runs/9957713967/job/27510405428#logs))

Not running external-http tests. To execute these, use --group external-http.

Deprecated: Using ${var} in strings is deprecated, use {$var} instead in /var/www/tests/phpunit/tests/html-api/wpHtmlProcessorHtml5lib.php on line 257
PHPUnit 9.6.20 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.21

After this patch, the deprecation is no longer thrown ✅

Marking for and preparing the commit.

#15 @hellofromTonya
7 days ago

In 58733:

HTML API: Fix "${var} in strings" deprecation notice in html5lib test.

Changeset [58712] introduced the following compile time PHP deprecation notice on >= PHP 8.2 test runs:

Deprecated: Using ${var} in strings is deprecated, use {$var} instead in /var/www/tests/phpunit/tests/html-api/wpHtmlProcessorHtml5lib.php on line 257
PHPUnit 9.6.20 by Sebastian Bergmann and contributors.

The ${ syntax for string interpolation was deprecated in PHP 8.2 and should not be used anymore.

Ref: https://wiki.php.net/rfc/deprecate_dollar_brace_string_interpolation

Follow-up to [58712].

Props jrf.
See #61530, #59654, #61576.

This ticket was mentioned in PR #7040 on WordPress/wordpress-develop by @jrf.


7 days ago
#17

### Tests/wpInteractivityAPIFunctions: don't declare nested named function

Once the test_process_directives_when_block_is_filtered() method has run, the named test_render_block_data() function declared nested within becomes part of the global namespace, which could cause problems for other tests.

Quite apart from the fact that the name starting with test_ is confusing (as methods prefixed with test_ are supposed to be test methods to be run by PHPUnit).

Using a closure for this callback fixes the issue.

### Tests/test_wp_interactivity_data_wp_context_with_different_arrays(): use data provider

Refactor the test to use a data provider with named test cases.

This is better as:

  1. One failing test will not block the other tests from running.
  2. Each test is now referenced by name in any error message, making it more straight forward to see which test failed.
  3. The test no longer contains multiple assertions.
  4. It makes it more straight forward to add additional tests.

### Tests/test_wp_interactivity_data_wp_context_with_different_arrays_and_a_namespace(): use data provider

Refactor the test to use a data provider with named test cases.

This is better as:

  1. One failing test will not block the other tests from running.
  2. Each test is now referenced by name in any error message, making it more straight forward to see which test failed.
  3. The test no longer contains multiple assertions.
  4. It makes it more straight forward to add additional tests.

### Tests/test_wp_interactivity_data_wp_context_with_json_flags(): use data provider

Refactor the test to use a data provider with named test cases.

This is better as:

  1. One failing test will not block the other tests from running.
  2. Each test is now referenced by name in any error message, making it more straight forward to see which test failed.
  3. The test no longer contains multiple assertions.
  4. It makes it more straight forward to add additional tests.

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

#18 follow-up: @jrf
7 days ago

PR GH 7040 contains some stability and maintainability improvements for the InteractivityAPI tests.

#19 @dmsnell
7 days ago

In 58734:

HTML API: Add tests confirming comment behavior in HTML Processor.

There was a bug-fix late in the 6.6 cycle in the HTML Processor which
resolved an issue with the wrong name being reported when paused at
processing-instruction look-alike invalid comments, but no tests were
added to enforce the correct behaviors.

This patch adds the missing tests.

Developed in https://github.com/WordPress/wordpress-develop/pull/6765
Discussed in https://core.trac.wordpress.org/ticket/61530

Follow-up to [58304], [58558].

Props dmsnell, jonsurrell.
See #61530.

This ticket was mentioned in PR #6765 on WordPress/wordpress-develop by @jonsurrell.


7 days ago
#21

Trac ticket: Core-61530

Add tests for comment handling.

These tests caught a regression during the 6.6 development cycle (with processing instruction comment tag name) that was fixed, but the tests were not merged.

#22 in reply to: ↑ 18 @hellofromTonya
7 days ago

Replying to jrf:

PR GH 7040 contains some stability and maintainability improvements for the InteractivityAPI tests.

Reviewed. Approved. In the process of prepping the commit.

#23 @hellofromTonya
7 days ago

In 58738:

Tests: Don't declare nested named function in Tests_Interactivity_API_wpInteractivityAPIFunctions.

Once the test_process_directives_when_block_is_filtered() method has run, the named test_render_block_data() function declared nested within becomes part of the global namespace, which could cause problems for other tests.

Quite apart from the fact that the name starting with test_ is confusing (as methods prefixed with test_ are supposed to be test methods to be run by PHPUnit).

Using a closure for this callback fixes the issue. Declared as static for a micro-optimization.

Follow-up to [57826].

Props jrf, hellofromTonya.
See #61530.

#24 @hellofromTonya
7 days ago

In 58739:

Tests: Use data provider in Tests_Interactivity_API_wpInteractivityAPIFunctions.

Refactors the following tests to use a data provider with named test cases:

  • test_wp_interactivity_data_wp_context_with_different_arrays()
  • test_wp_interactivity_data_wp_context_with_different_arrays_and_a_namespace()
  • test_wp_interactivity_data_wp_context_with_json_flags()

This is better as:

  1. One failing test will not block the other tests from running.
  2. Each test is now referenced by name in any error message, making it more straight forward to see which test failed.
  3. The test no longer contains multiple assertions.
  4. It makes it more straight forward to add additional tests.

Follow-up to [58594], [58234], [57762], [57743], [57742], [57563].

Props jrf.
See #61530.

@jrf commented on PR #7040:


7 days ago
#26

Thanks @hellofromtonya!

Note: See TracTickets for help on using tickets.