Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#54725 closed task (blessed) (fixed)

Test tool and unit test improvements for 6.0

Reported by: hellofromtonya's profile hellofromTonya Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.0 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch has-unit-tests fixed-major dev-reviewed commit
Focuses: Cc:

Description

Previously:

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.

Attachments (4)

54725.diff (4.3 KB) - added by azouamauriac 3 years ago.
Use data provider tests/phpunit/tests/functions/wpToKebabCase.php; typo in tests/phpunit/tests/functions/wpValidateBoolean.php
54725.2.diff (479 bytes) - added by azouamauriac 3 years ago.
Add docblock
54725.3.diff (432 bytes) - added by azouamauriac 3 years ago.
introduced here [52797]
54725.4.diff (734 bytes) - added by azouamauriac 3 years ago.
Remove some unused variables; some history: [31014]; [29422]

Download all attachments as: .zip

Change History (46)

This ticket was mentioned in PR #1995 on WordPress/wordpress-develop by johnbillion.


3 years ago
#1

  • Keywords has-patch has-unit-tests added

Several tests perform assertions conditionally or iterate dynamic arrays without ensuring they're populated. If the test is faulty and the condition never evaluates to true or the array being iterated is empty then we'll never know about it.

This adds additional assertions that cover these situations.

Update: Moving this PR to the new 6.0 ticket.
Trac ticket: https://core.trac.wordpress.org/ticket/54725

johnbillion commented on PR #1995:


3 years ago
#2

Thanks for the comments @jrfnl , I've pushed some changes. I've left the whitespace unadjusted for now to keep the diff clean, the whitespace can be corrected at the point of commit.

Next up, we've actually got some failures as a result of these additional assertions that need investigating.

jrfnl commented on PR #1995:


3 years ago
#3

Next up, we've actually got some failures as a result of these additional assertions that need investigating.

Nah, not test failures: test successes as making these changes has probably uncovered some bugs in the underlying code 😁

#4 @johnbillion
3 years ago

In 52654:

Build/Test Tools: Switch to some more appropriate assertions.

See #54725

#5 @SergeyBiryukov
3 years ago

In 52773:

Tests: Correct the order of expected and actual values in get_status_header_desc() tests.

This corrects the order of the parameters when used in assertions so if/when they fail the failure message is correct.

Follow-up to [46107].

See #54725.

#6 @SergeyBiryukov
3 years ago

In 52774:

Tests: Correct the order of expected and actual values in wp_array_slice_assoc() tests.

This corrects the order of the parameters when used in assertions so if/when they fail the failure message is correct.

Follow-up to [45371].

See #54725.

#7 @SergeyBiryukov
3 years ago

In 52775:

Tests: Correct the order of expected and actual values in wp_validate_boolean() tests.

This corrects the order of the parameters when used in assertions so if/when they fail the failure message is correct.

Additionally, this commit moves the test function before the data provider, for consistency with other tests.

Follow-up to [46159], [46224].

See #54725.

#8 @SergeyBiryukov
3 years ago

In 52776:

Docs: Improve some DocBlocks in wp_validate_boolean() tests for consistency.

Follow-up to [46159], [46224], [52775].

See #54725, #54729.

#9 @SergeyBiryukov
3 years ago

In 52777:

Docs: Correct parameter types for data_wp_validate_boolean().

Follow-up to [46159], [46224], [52775], [52776].

See #54725, #54729.

#10 follow-up: @SergeyBiryukov
3 years ago

In 52778:

Tests: Convert _wp_to_kebab_case() tests to use a data provider.

Follow-up to [51198], [51225].

See #54725.

@azouamauriac
3 years ago

Use data provider tests/phpunit/tests/functions/wpToKebabCase.php; typo in tests/phpunit/tests/functions/wpValidateBoolean.php

#11 in reply to: ↑ 10 ; follow-up: @azouamauriac
3 years ago

Replying to SergeyBiryukov:

In 52778:

Tests: Convert _wp_to_kebab_case() tests to use a data provider.

Follow-up to [51198], [51225].

See #54725.

wooops! I didn't know and i submit new patch again in 54725.diff, damn it(sorry)!

but there are some typo i've fixed in functions docblock. You should take a look. thanks.

#12 in reply to: ↑ 11 @SergeyBiryukov
3 years ago

Replying to azouamauriac:

wooops! I didn't know and i submit new patch again in 54725.diff, damn it(sorry)!

but there are some typo i've fixed in functions docblock. You should take a look. thanks.

Ah, sorry, I didn't know someone else was working on it. Looking at the DocBlocks, it seems like we're not very consistent with using third-person singular verbs in these test descriptions, so let's fix that at least in a few other files.

#13 @SergeyBiryukov
3 years ago

In 52780:

Docs: Use third-person singular verbs in some test descriptions in phpunit/tests/functions/.

Follow-up to [42971], [45371], [46159], [46175], [47779], [50962], [50964], [51910], [52778].

Props azouamauriac, SergeyBiryukov.
See #54725.

@azouamauriac
3 years ago

Add docblock

#14 @SergeyBiryukov
3 years ago

In 52798:

Tests: Add a @ticket reference for the current_theme_supports-{$feature} filter test.

Follow-up to [19682], [495/tests].

Props azouamauriac.
See #54725.

@azouamauriac
3 years ago

introduced here [52797]

@azouamauriac
3 years ago

Remove some unused variables; some history: [31014]; [29422]

#15 @peterwilsoncc
3 years ago

In 52938:

Tests: Include special characters in term names for wp_set_term_objects().

Test wp_set_term_objects() using terms with special characters in the name, for example ampersand, bullet and other symbols and punctuation.

Props kapacity, costdev.
Fixes #53152.
See #54725.

#16 @spacedmonkey
3 years ago

In 52976:

Build/Test Tools: Make comment cache group persistent in object-cache.php.

After the comment cache group was made persistent in [37613], also make
persistent in object-cache.php used for PHPUnit tests.

See #54725

This ticket was mentioned in PR #2453 on WordPress/wordpress-develop by jrfnl.


3 years ago
#17

Started this branch to debug a couple of tests (webp related) which are failing on PHP 5.6 when NOT run inside the Docker container, using a standard PHP 5.6 install.

This generally would indicate a missing @requires tag or missing skip condition.

I've done a large test refactor for that particular test class as nearly all methods were testing using foreach() loops instead of data providers, so it was impossible to see what was really failing.

While looking into this I found that the following curiousities:

  1. A function which was changed in WP 3.5.0 wp_save_image_file(), apparently also changed return type - from bool to array|WP_Error (BC-break anyone ?) -, but the documentation does not reflect that.
    • Does nobody use the function ?
    • Or if they do, how can it be that nobody noticed this in 25 major releases ?
  2. The failing tests are in part not actually testing what they should be testing and if tested this way, there seems to be some dependency on a particular (not necessarily standard) PHP compilation. I haven't been able to track down the exact difference or rather how to detect the difference, but the result is the failing tests.

Note: the last commit is very much WIP/debugging. The commits before that are valid improvements which can be (and should be) committed anyway.

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

jrfnl commented on PR #2453:


3 years ago
#18

Documentation fix commit belongs with #54729

#19 @jrf
3 years ago

I've opened DRAFT PR https://github.com/WordPress/wordpress-develop/pull/2453

Most changes in that PR are ready for commit, except for the "WIP/DEBUG" commit at the top of the stack.

See the PR description for additional context.

There is one documentation fix commit in the chain which belong with #54729

#20 @SergeyBiryukov
3 years ago

In 52983:

Tests: Remove some unused variables in phpunit/tests/filters.php.

Follow-up to [29422], [31014].

Props azouamauriac.
See #54725.

#21 @SergeyBiryukov
3 years ago

In 52984:

Docs: Adjust documentation in get_post_galleries() tests per the documentation standards.

Follow-up to [52190], [52797].

Props azouamauriac, SergeyBiryukov.
See #54725, #54729.

jrfnl commented on PR #2453:


3 years ago
#22

I've updated the third commit - "Add helper function for creating named data provider" - as discussed with @hellofromtonya in today's call:

  • Moved the function from this particular test class to the generic WP_UnitTestCase_Base class.
  • Added an extra safeguard to prevent duplicate entries overwriting each other.

Other than that, I've added an extra commit at the top of the commit chain with the additional debug code used in today's call with @adamsilverstein and @hellofromtonya .

As discussed, the first 13 commits can, and should, be committed as soon as the have been reviewed and found to be okay.

The last two commits will then remain as helpers to solve the underlying issue now identified with webP support on PHP 5.6/7.0.

jrfnl commented on PR #2453:


3 years ago
#23

Ha, just noticed I'd forgotten the @since tag for the new test helper, so updated that commit.

I've now pushed the first 13 commits again to show a passing build for those, so the committer can see those commits are "clean" and ready to go.
Once that build has finished, I will add the last two commits again.

#24 @peterwilsoncc
3 years ago

In 53033:

Build/Test Tools: Add unit tests for feed_links_extra().

Props costdev, hellofromTonya.
Fixes #54713.
See #54725.

#26 @desrosj
3 years ago

In 53112:

Build/Test Tools: Update all 3rd party actions to their latest versions.

This updates all 3rd party GitHub actions to their latest versions.

  • actions/cache from 2.1.6 to 3.0.1.
  • actions/github-script from 5.0.0 to 6.0.0.
  • actions/setup-node from 2.4.1 to 3.1.0.
  • codecov/codecov-action from 2.1.0 to 3.0.0.
  • ramsey/composer-install from 1.3.0 to 2.1.0.
  • shivammathur/setup-php from 2.15.0 to 2.18.0.

Additionally, this updates all instances of the actions/setup-node action to replace the node-version option with the new node-version-file. This simplifies the process of changing the version of NodeJS used in workflows by only requiring the version to be changed once in the .nvmrc file.

See #54725.

#28 @SergeyBiryukov
3 years ago

In 53319:

Tests: Ignore EOL differences in Webfonts API tests.

Unix vs. Windows EOL style mismatches can cause misleading failures in tests using the heredoc syntax (<<<) or multiline strings as the expected result.

Includes renaming the test class to match the naming conventions.

Follow-up to [46612], [48443], [48466], [49691], [51135], [53282].

See #54725.

#29 @SergeyBiryukov
3 years ago

In 53322:

Coding Standards: Remove extra alignment level in the data provider for wp_validate_boolean() tests.

Follow-up to [46159], [46224], [52775], [52776], [52777].

See #54725, #54728.

#30 @hellofromTonya
3 years ago

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

Opened #55652 ticket for tracking ongoing continuous test tooling and suite improvements for the 6.1 cycle.

With 6.0 RC1 tomorrow, closing this ticket. Feel free to reopen if committing before RC1 release party starts, else use #55652.

This ticket was mentioned in PR #2701 on WordPress/wordpress-develop by hellofromtonya.


3 years ago
#31

The odd / even class attribute global variables are causing issues in comments tests when a new test is added or an existing test is modified. To stabilize the odd / even comment tests, the comment global variables are added to the base test class' tear_down() using the same patterns as the other global resets. This change ensures each comment test starts at the same state. In doing so, the expected odd / even class attributes are no longer affected by previous tests, i.e. test leaks.

Adding this to a 6.0 test improvement ticket for backport to 6.0-branch since this issue was identified in the 6.0 cycle.

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

#32 @hellofromTonya
3 years ago

In 53430:

Build/Test Tools: Fix comments odd/even instabilities (test leaks).

The odd / even class attribute global variables are causing issues in comments tests when a new test is added or an existing test is modified. To stabilize the odd / even comment tests, the comment global variables are added to the base test class' tear_down() using the same patterns as the other global resets. This change ensures each comment test starts at the same state. In doing so, the expected odd / even class attributes are no longer affected by previous tests, i.e. test leaks.

Follow-up to [53172].

Props hellofromTonya, zieladam, peterwilsoncc.
See #54725.

#33 @hellofromTonya
3 years ago

  • Keywords fixed-major dev-reviewed commit added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to backport [53430] to 6.0 branch, ie where the original issue was found.

This ticket was mentioned in PR #2742 on WordPress/wordpress-develop by hellofromtonya.


3 years ago
#34

Backports changeset [53353] and [53430] + moves the global resets from WP_UnitTestCase_Base::tear_down() to the test's tear_down() to avoid potential issues for extenders (see @peterwilsoncc comment https://github.com/WordPress/wordpress-develop/pull/2701#discussion_r870830662].

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

#35 @hellofromTonya
3 years ago

  • Owner set to hellofromTonya
  • Resolution set to fixed
  • Status changed from reopened to closed

In 53432:

Build/Test Tools: Add tests and fix comments odd/even instabilities (test leaks).

[53353] Add unit test for Comment Template block.

[53353] The odd / even class attribute global variables are causing issues in comments tests when a new test is added or an existing test is modified. To stabilize the odd / even comment tests, the comment global variables are added to the base test class' tear_down() using the same patterns as the other global resets. This change ensures each comment test starts at the same state. In doing so, the expected odd / even class attributes are no longer affected by previous tests, i.e. test leaks.

Also moves the comment globals reset from the base test case to the test's tear_down(). Why? To avoid risks to extenders' tests as it's too late in the 6.0 cycle for a dev note.

Follow-up to [53298], [53172], [53138].

Props bernhard-reiter, darerodz, gziolo, hellofromTonya, zieladam, peterwilsoncc.
Merges [53353] and [53430] to the 6.0 branch.
Fixes #54725,#55643.

#36 @hellofromTonya
3 years ago

In 53434:

Build/Test Tools: Follow-up to r53432.

Removes comments global resets from base test case.

Follow-up to [53432].

See #54725,#55643.

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


3 years ago

jrfnl commented on PR #2453:


3 years ago
#40

@SergeyBiryukov Shall I rebase the PR ?

SergeyBiryukov commented on PR #2453:


3 years ago
#41

Shall I rebase the PR ?

Not sure if that's necessary, the test improvement commits still seem to apply correctly :)

jrfnl commented on PR #2453:


3 years ago
#42

@SergeyBiryukov Fair enough, I'm just allergic to back-merges in PRs ;-)

Note: See TracTickets for help on using tickets.