#58955 Promote some test skipping to failures #5100
Conversation
johnbillion
changed the title
johnbillion
changed the title
There was a problem hiding this comment.
@johnbillion I'm not sure what is going on here, but the majority of these changes do not seem correct.
| $this->markTestSkipped( 'This test requires the XSL extension.' ); | ||
| $this->fail( 'This test requires the XSL extension.' ); |
There was a problem hiding this comment.
Why should the tests fail when the extension is missing ? A contributor could be running the tests locally and not have the extension enabled. Marking a test as skipped in such a case is the appropriate way to handle this. Not failing the test suite.
| @@ -69,7 +69,7 @@ function( $args ) use ( $urls ) { | |||
| $has_failed = ! empty( $failed ); | |||
|
|
|||
| if ( ! $has_successful && ! $has_failed ) { | |||
| $this->markTestSkipped( 'This test requires at least one successful or failed plugin update object.' ); | |||
| $this->fail( 'This test requires at least one successful or failed plugin update object.' ); | |||
There was a problem hiding this comment.
As far as I can see, this should be handled via a @depends tag, not via test skipping/failing.
Tests are not guaranteed to run in the same order and can even be explicitly requested to be run in a different order using --order-by. This test could now cause random test failures based on the test execution order.
As a side-note: running tests in random order is a great way to find tests which are too closely coupled or don't do their setup/teardown correctly.
| $this->markTestSkipped( 'This test requires a switch_to_blog() method on the cache object.' ); | ||
| $this->fail( 'This test requires a switch_to_blog() method on the cache object.' ); |
There was a problem hiding this comment.
Is this even a realistic scenario for a test ? I.e. how can it happen that a cache object created for this test would not have that method ? And if the method doesn't exist, wouldn't test fail no matter what, so why does the condition exist ?
| $this->markTestSkipped( 'No European locales available for testing.' ); | ||
| $this->fail( 'No European locales available for testing.' ); |
There was a problem hiding this comment.
Again: A contributor could be running the tests locally and not have the European locale available. Marking a test as skipped in such a case is the appropriate way to handle this. Not failing the test suite.
| $this->markTestSkipped( 'This system does not support Signature Verification.' ); | ||
| $this->fail( 'This system does not support Signature Verification.' ); |
There was a problem hiding this comment.
Same thing again. Failing tests should only be done when the code under test doesn't run correctly. Not when the environment in which the tests are run does not comply with the desired environment.
| @@ -71,7 +71,7 @@ public function set_up() { | |||
| $this->export_file_name = ''; | |||
|
|
|||
| if ( ! $this->remove_exports_dir() ) { | |||
| $this->markTestSkipped( 'Existing exports directory could not be removed. Skipping test.' ); | |||
| $this->fail( 'Existing exports directory could not be removed. Skipping test.' ); | |||
There was a problem hiding this comment.
| $this->fail( 'Existing exports directory could not be removed. Skipping test.' ); | |
| $this->fail( 'Existing exports directory could not be removed.' ); |
Though, again, I don't think failing the test is correct here. There may be contributor system specific reasons why the directory could not be removed.
| $this->markTestSkipped( 'Limiting fields requires the WP_REST_Controller::get_fields_for_response() method.' ); | ||
| $this->fail( 'Limiting fields requires the WP_REST_Controller::get_fields_for_response() method.' ); |
There was a problem hiding this comment.
I think this needs to be reverted.
The only reason I can imagine that this condition exists, is to make backports of this test easier if the WP_REST_Controller class in an earlier iteration didn't have the get_fields_for_response() method.
If that is not the reason for the condition, then I'd say the whole condition block should likely be removed as the tests should fail without the method anyway.
| $this->markTestSkipped( 'Limiting fields requires the WP_REST_Controller::get_fields_for_response() method.' ); | ||
| $this->fail( 'Limiting fields requires the WP_REST_Controller::get_fields_for_response() method.' ); |
| $this->markTestSkipped( "$block_theme must be available." ); | ||
| $this->fail( "$block_theme must be available." ); |
There was a problem hiding this comment.
Same thing again. Failing tests should only be done when the code under test doesn't run correctly. Not when the environment in which the tests are run does not comply with the desired environment.
| $this->markTestSkipped( "Could not switch to $block_theme." ); | ||
| $this->fail( "Could not switch to $block_theme." ); |
There was a problem hiding this comment.
Same thing again. Failing tests should only be done when the code under test doesn't run correctly. Not when the environment in which the tests are run does not comply with the desired environment.
Trac ticket:
https://core.trac.wordpress.org/ticket/60705
https://core.trac.wordpress.org/ticket/59647
https://core.trac.wordpress.org/ticket/58955
This changes several instances of test skipping to test failures, and removes some unnecessary
function_exists()checks for compat functions.If any of the checks in these test aren't satisfied then the test should fail rather than be skipped.
In addition, I removed some multisite test skipping which is redundant as it's handled by the
ms-requiredandms-excludedtest group handling.Previously:
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.