Readme Check: Compare Tested up to value with latest stable version

[ernilambar]

Fixes #481

[ernilambar]

Refinement in Unit tests:

We could remove over-scoping of test methods so that we would not have to update tests in every readme related PR.

Example:

1) Plugin_Readme_Check_Tests::test_run_with_errors_invalid_name
Failed asserting that 2 matches expected 1.

When we are testing ERROR for invalid plugin name, we should not be caring how many warnings are there. We should focus on whether our ERROR is there or not.

Related: https://github.com/WordPress/plugin-check/actions/runs/9743511198/job/26887103212#step:7:270

[swissspidy]

Agreed, I don't know why all these tests are doing that.

[davidperezgar]

I've made some changes: Detect major version (for example 6.5) and prevent some readme file without version added.

[swissspidy]

I've made some changes: Detect major version (for example 6.5) and prevent some readme file without version added.

Let's undo the changes to get_wordpress_stable_version.

We should rely on https://api.wordpress.org/core/version-check/1.7/ providing a correct value for new_bundled, which is always the latest major (e.g. 6.5).

(Note: for some reason it says 6.4 right now but I already reported this bug).

[ernilambar]

I refactored the test test_run_with_errors_invalid_name like this:

public function test_run_with_errors_invalid_name() {
    $readme_check  = new Plugin_Readme_Check();
    $check_context = new Check_Context( UNIT_TESTS_PLUGIN_DIR . 'test-plugin-plugin-readme-errors-invalid-name/load.php' );
    $check_result  = new Check_Result( $check_context );

    $readme_check->run( $check_result );

    $errors = $check_result->get_errors();

    $this->assertNotEmpty( $errors );
    $this->assertArrayHasKey( 'readme.txt', $errors );

    // Check for invalid name error.
    $this->assertArrayHasKey( 0, $errors['readme.txt'] );
    $this->assertArrayHasKey( 0, $errors['readme.txt'][0] );
    $this->assertSame( 1, count( wp_list_filter( $errors['readme.txt'][0][0], array( 'code' => 'invalid_plugin_name' ) ) ) );
}

Here:

• I have removed assertions related to warnings
• Also for error, I am not considering it is in either 0 or 1 position because position of the error could change depending upon order of methods $this->check_name() and $this->check_headers called.

Errors array:

(
    [readme.txt] => Array
        (
            [0] => Array
                (
                    [0] => Array
                        (
                            [0] => Array
                                (
                                    [message] => Plugin name header in your readme is missing or invalid. Please update your readme with a valid plugin name header. Eg: "=== Example Name ==="
                                    [code] => invalid_plugin_name
                                    [link] => 
                                )

                            [1] => Array
                                (
                                    [message] => Tested up to: 6.1 < 6.5
                                    [code] => outdated_tested_upto_header
                                    [link] => 
                                )

                        )

                )

        )

)

Is this approach good?

CC @swissspidy @mukeshpanchal27

[swissspidy]

You can use assertCount() to be more precise, instead of assertSame( 1, count(

[swissspidy]

In the future we could think about custom PHPUnit assertions to make our lives easier.

[ernilambar]

In the future we could think about custom PHPUnit assertions to make our lives easier.

I tried this approach also. https://github.com/ernilambar/plugin-check/pull/10/files

[ernilambar]

@swissspidy For tests like public function test_run_without_errors(), how can we update test such that we wont have to fix test when we add new check giving error/warning?

[swissspidy]

You mean specifically for \Plugin_Readme_Check_Tests::test_run_without_errors()? Well, it depends.

Given that this PR updates the check and the readme.txt used in tests (tests/phpunit/testdata/plugins/test-plugin-plugin-readme-without-errors/readme.txt) is very basic, it makes sense that the test now fails.

If you mean specifically for the Tested up to field in that file, we could think about some automation to keep that up-to-date with the latest WP version. But that can be discussed in a new issue.

[ernilambar]

Not specific to Tested up to. Lets say in the future Short Description will be required. Then these tests like test_run_without_errors, test_run_md_without_errors, test_run_root_readme_file_without_errors, etc will fail.

What if have an fixed array of codes which we check against. Though, in that case test_run_without_errors name wont be appropriate coz we will be just checking few codes only.

[swissspidy]

There's definitely room for improvement in how tests are structured and how we check for error codes.

And tests like "runs without errors" are a bit too broad. Better to check for good and bad cases for each individual sub-check.

But let's discuss that in a new issue :-)

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ernilambar <rabmalin@git.wordpress.org>
Co-authored-by: davidperezgar <davidperez@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[davidperezgar]

What happen with the version? We keep regex then?

[swissspidy]

Yes. Turns out I was wrong and we have to keep using current

[davidperezgar]

I see everything is OK.