test_naming.md

• The inclusion of _it_ seems unnecessary in the context of these isolated tests, UNLESS additional clarity is desired, in which case this should include the function name under test, which seems pretty common.

it is often used in JS tests/specs in the sense:

describe( 'disableBlockEditorForNavigationPostType', () => {
    // Some suites use "it"
    it( 'should only disable block editor for navigation post types',      () => {})
    // Some suites use "test"
    test( 'it should only disable block editor for navigation post types', () => {})
    // For suites using "test", removing "it" is common
    test( 'should only disable block editor for navigation post types',    () => {})
})

Converted to PHPUnit, the third example above would look like:

class Tests_Editor_DisableBlockEditorForNavigationPostType extends WP_UnitTestCase {
    public function test_should_only_disable_block_editor_for_navigation_post_types() {
    }
}

Each of these migrated tests have the same test method name, test_it_correctly_handles_different_post_types(), which seems awkward compared with others in the project. Maybe it's just me, but an alternative might be to simplify it to test_handle_different_post_types().

My issue with handle - and admittedly, I've used it myself in the past, is that it doesn't describe the expected behaviour.

i.e. test_handle_different_post_types() vs test_should_only_disable_block_editor_for_navigation_post_types().

By using test_should_ and test_should_not_ prefixes where possible, that means:

• Raw output:

Tests_Editor_DisableBlockEditorForNavigationPostType::test_should_only_disable_block_editor_for_navigation_post_types
Tests_Editor_DisableBlockEditorForNavigationPostType::test_should_not_disable_block_editor_for_missing_post_types

• --testdox output:

_Editor_DisableBlockEditorForNavigationPostType
 ✔ Should only disable block editor for navigation post types
 ✔ Should not disable block editor for missing post types

Theoretically, _should_ may not be necessary, but it gives a clear indication of the expected result before the reader even knows the remaining context, and we'll often internally say "Okay, this function should only / should not do something", so this saves the reader time.

Thanks, @costdev! I originally had a much longer comment, too (including sample --testdox output!), and stripped it down quite a bit, because it was getting really pedantic, a sin that I struggle to avoid daily 😅 The gist is a good alternative for discussion.

The non-unique function names, and use of "it" and "correctly", were what I had the most trouble grappling with. The JS test context you provide makes sense, but rubs up against what I've seen in existing PHPUnit tests in the project, hence the raised flag (...in the context of someone who's only been on the core project for ~8 months, so grain of salt and all).

I do like the examples you provided, which are much more precise as to the intent and what's happening with the tests, and amend my suggestions along those lines. What do you and @anton-vlasenko think about these?

For editor/disableBlockEditorForNavigationPostType.php:

• test_should_only_disable_block_editor_for_navigation_post_types()
• test_should_not_disable_block_editor_for_non_navigation_post_types()

editor/disableContentEditorForNavigationPostType.php:

• test_should_only_disable_content_editor_for_navigation_post_types()
• test_should_not_disable_content_editor_for_non_navigation_post_types()

editor/enableContentEditorForNavigationPostType.php():

• test_should_only_enable_content_editor_for_navigation_post_types()
• test_should_not_enable_content_editor_for_non_navigation_post_types()

I swapped "missing" for "non-navigation" since the former implies something else if you were to say "a missing post type".

And for kses/wpFilterGlobalStylesPost.php:

• test_should_not_remove_safe_global_style_rules()
• [new] test_should_only_remove_unsafe_global_style_rules() <-- Does it make sense for us to add an opposing test to validate that the function does filter out unsafe rules?

Certainly test_it doesn't belong in the PHPUnit tests - Both IMO and in the fact that there are only four tests in the project that start with test_it_. I think those should be considered for changes either in the umbrella Tests ticket for 6.1, for example, or as part of another effort.

Thinking on the method prefixes again, I think test_should and test_should_not are appropriate prefixes. test_should_only is a little problematic as it mixes the should and should not testing concerns.

• Also, when testing class methods, unless we plan to have a separate file for each method, I think it's fine to use test_method_name_should... to identify which method is being tested.
• Side note: The should and behaviour/outcome approach to naming can be seen in Writing PHPUnit Tests - Test Methods section, so this wouldn't need any updates to the handbook.

On kses/wpFilterGlobalStylesPost.php:

• I'm inclined to say yes, it's worth having both. Why? Because it's quite possible that if edge cases are encountered, should_remove_unsafe... will have a data provider of rules considered unsafe, and should_not_remove_safe... will have a data provider of rules considered safe. Consolidating these into one test method with a potentially huge data provider would make it harder to digest.

Finally: On you mentioning pedantry, IMO, that's important when implementing, enforcing, or establishing standards. 🙂 Much like brainstorming, get it all out and filter it down to what's appropriate.

The non-unique function names, and use of "it" and "correctly", were what I had the most trouble grappling with.

@ironprogrammer I don't see an issue with non-unique test method names.
IMO, modern IDEs make it easy to understand which class a particular method belongs to.
But maybe it's just me.

Side note: The should and behaviour/outcome approach to naming can be seen in Writing PHPUnit Tests - Test Methods section, so this wouldn't need any updates to the handbook.

As long as should and should_not are in the documentation, I don't mind renaming the test methods. My only request would be to explicitly add to the documentation that using these prefixes for test method names is recommended. I've read the documentation, and it wasn't clear until @costdev emphasized it. Thanks, @costdev!

Does it make sense for us to add an opposing test to validate that the function does filter out unsafe rules?

Yes, I will do that.

+1 on the suggestion to use test_should_ and test_should_not_. Also, thanks, @costdev for pointing out the existing docs guideline concerning test names.

@anton-vlasenko, I agree the naming convention could be made more obvious in the docs. While no consensus/handbook update is necessary for us to use the test_should_ convention here, I think it's worth looking into a future docs update that recommends the converse test_should_not_, as well as points out the clearly established convention for including the method name (particularly in combined test class files) that @costdev alluded to. I'm working on Test Handbook updates that could incorporate this.

IMO, modern IDEs make it easy to understand which class a particular method belongs to.

@anton-vlasenko It's mainly for clarity that we can make the test method names as unique as possible. When running tests locally or in CI/CD, the self-descriptive names could save time in understanding the failure 🙂.

Just an update that I've made this Gist public after realising that I had created it as a "Secret" Gist - My bad.

It's mainly for clarity that we can make the test method names as unique as possible. When running tests locally or in CI/CD, the self-descriptive names could save time in understanding the failure 🙂.

IMO, since the class name already shows in the list of failures, that's not an issue for me.

However, running a specific test becomes a little more awkward via CLI.

Consider:

phpunit --filter should_return_false_for_multiple_post_types

It's quite possible that this test method name could be used in multiple test classes.

So, you would either have to have an IDE that allows you to click to run a specific test, or use one of the following:

# Meaning manually typing the class name or copying/pasting the class and method names separately.
phpunit --filter Test_Class_Name::should_return_false_for_multiple_post_types

# Not ideal.
phpunit --group <ticket_no> --filter should_return_false_for_multiple_post_types

# Not possible due to file name conventions not matching test class names in a way that PHPUnit understands (I don't know if there's a way around this already?)
phpunit /path/to/test/file.php

On a personal note, I prefer just using should_return_false_for_multiple_post_types without the tested method name, but I can't think of a way that isn't more difficult when running tests.

I've renamed the test methods.
But I didn't create test_should_not methods.
Why?
That would lead to code duplication.
The test classes would need to have the same code for test_should and test_should_not methods.
If the intention is to save time in understanding the failure, I think we are fine because these particular tests use named data sets and unique error messages.
@ironprogrammer @costdev Please review the code and let me know what you think.
WordPress/wordpress-develop#3005