Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improve Test Coverage for Webp-Uploads Plugin #1946

Draft
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

sarthak-19
Copy link
Contributor

Summary

This is part of #1789:

  • Ignore Coverage for Non-Critical Code Blocks
  • Add Missing @covers Annotations
  • Add Missing Tests

@sarthak-19
Copy link
Contributor Author

I've ignored the deprecated functions from code coverage.
Should these be removed completely or as of now should I ignore it only.

cc : @westonruter

@westonruter
Copy link
Member

@sarthak-19 good question. I'm not sure how deprecated they are.

@adamsilverstein are these on their way out or are they still important? I recall they were recently modified.

Copy link

codecov bot commented Mar 18, 2025

Codecov Report

Attention: Patch coverage is 0% with 286 lines in your changes missing coverage. Please review.

Project coverage is 64.25%. Comparing base (ad15727) to head (7009984).
Report is 181 commits behind head on trunk.

Files with missing lines Patch % Lines
plugins/webp-uploads/helper.php 0.00% 286 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk    #1946      +/-   ##
==========================================
- Coverage   70.99%   64.25%   -6.74%     
==========================================
  Files          85       83       -2     
  Lines        6958     6933      -25     
==========================================
- Hits         4940     4455     -485     
- Misses       2018     2478     +460     
Flag Coverage Δ
multisite 64.25% <0.00%> (-6.74%) ⬇️
single 32.48% <0.00%> (-8.45%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sarthak-19 sarthak-19 added this to the webp-uploads n.e.x.t milestone Mar 20, 2025
@sarthak-19 sarthak-19 added [Type] Enhancement A suggestion for improvement of an existing feature [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) skip changelog PRs that should not be mentioned in changelogs labels Mar 20, 2025
@sarthak-19
Copy link
Contributor Author

sarthak-19 commented Mar 20, 2025

  1. The deprecated functions are not being used anywhere in webp-uploads plugin, so is it safe to assume that we can remove them?

  2. The coverage of this plugin as reported by codecov is suddenly dropped to 0%.

Ref : https://app.codecov.io/gh/WordPress/performance/pull/1946/tree/plugins/webp-uploads?dropdown=coverage

However when I'm generating the coverage report with modified changes in local env. I'm getting 80% coverage.

Codecov Local
image image

Any ideas why this might be happening?

cc : @westonruter

@westonruter
Copy link
Member

1. The deprecated functions are not being used anywhere in webp-uploads plugin, so is it safe to assume that we can remove them?

I see that too. I guess they can be removed, but they're already ignored from code coverage, so I think we can consider removal in another PR.

2. The coverage of this plugin as reported by codecov is suddenly dropped to 0%.

Sorry, I don't know. Maybe @ShyamGadde has an idea.

Copy link
Contributor

@ShyamGadde ShyamGadde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sarthak-19 I'm curious about how you were able to generate the coverage reports locally. I tried replicating this on both my local environment and on a private performance repo I set up specifically for testing workflows, but couldn't get it to run.

Taking a closer look at the workflow logs, you can see that the coverage report for the webp-uploads plugin isn't being generated at all, which explains why the coverage is showing as zero.

*/

class Test_WebP_Uploads_Uninstall extends WP_UnitTestCase {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The culprit appears to be in the uninstall test for single site - the WP_UNINSTALL_PLUGIN constant isn't defined, which causes uninstall.php to exit early:

// If uninstall.php is not called by WordPress, bail.
if ( ! defined( 'WP_UNINSTALL_PLUGIN' ) ) {
exit;
}

This premature exit terminates the entire PHP runtime execution, preventing the coverage report from being generated. Interestingly, the test itself doesn't fail, which can be confusing at first.

A possible fix would be to define the constant in the test class like this:

Suggested change
/**
* Runs the routine before setting up all tests.
*/
public static function set_up_before_class(): void {
parent::set_up_before_class();
// Mock uninstall const.
if ( ! defined( 'WP_UNINSTALL_PLUGIN' ) ) {
define( 'WP_UNINSTALL_PLUGIN', 'Yes' );
}
}

/**
* Test uninstall on a single site.
*/
public function test_uninstall_single_site(): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the above change will solve the issue of the report not generating, there's still a potential problem with multisite tests. Since the single site test will also run and uninstall.php is included using include_once, it won't execute again for multisite tests, causing those assertions to fail.

To prevent this issue, you might want to add this check to the single site uninstall test:

Suggested change
public function test_uninstall_single_site(): void {
public function test_uninstall_single_site(): void {
if ( is_multisite() ) {
$this->markTestSkipped( 'This test is for single site only.' );
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ShyamGadde thank you so much for pointing me in the right direction.

I'm curious about how you were able to generate the coverage reports locally.

I suppose I was just testing for multisite in my local env.
npm run test-php-multisite:auto-sizes -- -- -- --coverage-html=./html-reports & probably that's why in my local the code coverage was showing up.

Thanks I'll make the necessary changes soon 🙇🏻

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
[Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) skip changelog PRs that should not be mentioned in changelogs [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants