Closed ernilambar closed 3 days ago
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
Agreed, I don't know why all these tests are doing that.
I've made some changes: Detect major version (for example 6.5) and prevent some readme file without version added.
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).
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:
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
You can use assertCount()
to be more precise, instead of assertSame( 1, count(
In the future we could think about custom PHPUnit assertions to make our lives easier.
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
@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?
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.
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.
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 :-)
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.
What happen with the version? We keep regex then?
Yes. Turns out I was wrong and we have to keep using current
Fixes https://github.com/WordPress/plugin-check/issues/481