Make WordPress Core

Opened 7 years ago

Last modified 2 years ago

#40538 accepted task (blessed)

Fix or remove useless PHPUnit tests

Reported by: johnbillion's profile johnbillion Owned by: johnbillion's profile johnbillion
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: needs-unit-tests ongoing has-patch
Focuses: Cc:

Description

There are 29 tests in the test suite which don't perform an assertion. They should be fixed or removed.

PHPUnit 6 has switched to being strict about useless tests by default, so that gives us an additional reason to address them. In addition, there's no reason for core's default PHPUnit configuration to not be strict about useless tests so the same behaviour is seen when running older versions of PHPUnit.

Previously: #36016

Attachments (2)

40538.diif (2.7 KB) - added by Mte90 7 years ago.
new 4 tests for rest api
rest-pages-controller.diff (1.4 KB) - added by tomepajk 6 years ago.
tests for WP_Test_REST_Pages_Controller::test_create_item and WP_Test_REST_Pages_Controller::test_get_item

Download all attachments as: .zip

Change History (15)

#1 @johnbillion
7 years ago

  • Owner set to johnbillion
  • Status changed from new to accepted

#2 @johnbillion
7 years ago

In 40534:

Build/Test Tools: Be strict about tests that do not test anything.

See #40538

#3 @johnbillion
7 years ago

In 40535:

Build/Test Tools: Ensure that WP_UnitTestCase::expectedDeprecated() performs an assertion to avoid risky test notices.

See #40538

#4 @johnbillion
7 years ago

In 40541:

Build/Test Tools: Only perform an assertion for deprecated calls and wrongdoings if any are expected.

This avoids masking risky tests that don't otherwise perform an assertion.

See #40538

#5 @johnbillion
7 years ago

In 40542:

Build/Test Tools: More tweaks to the deprecated calls assertion. This needs to be triggered when there are unexpected deprecated calls or wrongdoings too.

See #40538

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


7 years ago

#7 @obenland
7 years ago

@johnbillion Can this be closed for 4.8?

#8 @johnbillion
7 years ago

  • Keywords needs-unit-tests ongoing added
  • Milestone changed from 4.8 to Future Release

There are still a bunch of tests that don't perform any assertions.

@Mte90
7 years ago

new 4 tests for rest api

#9 @Mte90
7 years ago

  • Keywords has-patch added

This patch remove the empty units for 4 tests :-)

Are missing:

There were 4 risky tests:

1) WP_Test_REST_Pages_Controller::test_prepare_item
This test did not perform any assertions

2) WP_Test_REST_Settings_Controller::test_context_param
This test did not perform any assertions

3) WP_Test_REST_Settings_Controller::test_prepare_item
This test did not perform any assertions

4) WP_Test_REST_Settings_Controller::test_get_item_schema
This test did not perform any assertions

I am not sure about prepare, context and get_item_schema tests about what they have to do.

#10 @johnbillion
7 years ago

Thanks for the patch, @Mte90. AFAICT these tests are actually blocked by a bug I found when working on #41463. See 4:ticket:41463.

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


6 years ago

@tomepajk
6 years ago

tests for WP_Test_REST_Pages_Controller::test_create_item and WP_Test_REST_Pages_Controller::test_get_item

#12 @SergeyBiryukov
2 years ago

In 53921:

Tests: Consistently skip tests for non-implemented methods in REST API test classes.

WordPress core test suite uses PHPUnit's beStrictAboutTestsThatDoNotTestAnything option set to true, which marks a test as risky when no assertions are performed.

REST API test classes have some empty tests for non-implemented methods because these test classes extend the abstract WP_Test_REST_Controller_Testcase class, which requires several methods to be implemented that don't necessarily make sense for all REST API routes.

Some of these empty tests were already marked as skipped, but not in a consistent manner. Since skipping these tests is intentional for the time being, this commit aims to bring some consistency and adjust them all to be more accurately reported as skipped instead of risky.

The skipping can be reconsidered in the future when the tests are either removed as unnecessary or updated to actually perform assertions related to their behavior.

Follow-up to [40534], [41176], [41228].

Props Mte90, tomepajk, johnbillion, zieladam, SergeyBiryukov.
See #40538, #41463, #55652.

#13 @SergeyBiryukov
2 years ago

In 54058:

Tests: Explicitly mark empty REST API tests as not performing any assertions.

WordPress core test suite uses PHPUnit's beStrictAboutTestsThatDoNotTestAnything option set to true, which marks a test as risky when no assertions are performed.

REST API test classes have some empty tests for non-implemented methods because these test classes extend the abstract WP_Test_REST_Controller_Testcase class, which requires several methods to be implemented that don't necessarily make sense for all REST API routes.

As these tests are intentionally empty, they were previously marked as skipped, so that they are not reported as risky.

This commit aims to further reduce noise in the test suite and effectively ignores these empty tests altogether, which seems like a more appropriate option at this time.

The @doesNotPerformAssertions annotation can be reconsidered in the future when the tests are either removed as unnecessary or updated to actually perform assertions related to their behavior.

Follow-up to [40534], [41176], [41228], [53921].

See #40538, #41463, #55652.

Note: See TracTickets for help on using tickets.