Add PHPUnit tests for Image Loading Optimization#898

westonruter · 2023-12-06T01:21:44Z

Summary

This is part of #869.

This adds full test coverage for the functions in the Image Loading Optimization module.

Relevant technical choices

The ILO_HTML_Tag_Processor::open_tags() generator is updated to ensure accurate breadcrumbs/XPath for SVG and MathML elements.
The phpdoc and variable names are improved in ilo_construct_preload_links().
The integrity attribute is no longer copied from an IMG element to the preload LINK. This attribute doesn't appear on IMG, but rather is only on LINK and SCRIPT.
Rework how timestamps are added to URL metrics by adding and examining them in ilo_unshift_url_metrics(). Previously the timestamp was added in ilo_store_url_metric() before passing to ilo_construct_preload_links().
Refactor ilo_unshift_url_metrics() to make it easier to test by passing more arguments.
Fix JSON Schema definition in ilo_register_endpoint().
Discontinue excluding WordPress-Extra sniffs from applying to tests, and add specific sniff exclusions. By allowing WordPress-Extra to apply, PHPCBF can do automatic formatting. PHPCS errors were fixed in other test files. See Update PHPCS formatting in tests #924.
Fix REST API endpoint to reject storing a URL metric when the sample is already fully populated for the provided viewport. Previously it would continue to accept requests when any of the breakpoints lacked a full sample size of URL metrics.
Remove debug code from being returned in REST API response.

Checklist

PR has either [Focus] or Infrastructure label.
PR has a [Type] label.
PR has a milestone or the no milestone label.

…x issues

westonruter · 2023-12-08T23:05:59Z

modules/images/image-loading-optimization/class-ilo-html-tag-processor.php

 				// Immediately pop off self-closing tags.
-				if ( in_array( $tag_name, self::VOID_TAGS, true ) ) {
+				if (
+					in_array( $tag_name, self::VOID_TAGS, true )
+					||
+					( $p->has_self_closing_flag() && $this->is_foreign_element() )
+				) {
 					array_pop( $this->open_stack_tags );
 				}


@dmsnell: FYI, I'm pretty happy with how has_self_closing_flag and the foreign element aspect of the HTML spec simplified things! Now it can properly get breadcrumbs/XPaths for each SVG/MathML element as well.

…ty attr lifting

westonruter · 2023-12-13T23:57:11Z

modules/images/image-loading-optimization/optimization.php

+	$minimum_viewport_widths = array_keys( $lcp_elements_by_minimum_viewport_widths );
 	for ( $i = 0, $len = count( $minimum_viewport_widths ); $i < $len; $i++ ) {
-		$lcp_element = $lcp_images_by_minimum_viewport_widths[ $minimum_viewport_widths[ $i ] ];
-		if ( false === $lcp_element || empty( $lcp_element['attributes'] ) ) {


The empty( $lcp_element['attributes'] ) check is removed here because it is always set.

Another benefit of using a class to represent LCP elements would be that that would be truly enforced :)

westonruter · 2024-01-09T22:06:39Z

modules/images/image-loading-optimization/optimization.php

+	$needs_detection = in_array(
+		true,
+		// Each array item is array{int, bool}, with the second item being whether the viewport width is needed.
+		array_column( $needed_minimum_viewport_widths, 1 ),
+		true
+	);


Some cool things with PHPStan's static analysis here. If 0 is supplied as the second arg to array_column() it detects an error:

phpstan: Call to function in_array() with arguments true, list and true will always evaluate to false.

This is because from the array shape returned by ilo_get_needed_minimum_viewport_widths() it knows that the first column will all be integers (the minimum viewport widths) and thus the in_array() check will always be false.

Conversely, if I supply a higher index like 2 in array_column(), I get:

phpstan: Call to function in_array() with arguments true, array{} and true will always evaluate to false.

Here PHPStan knows that since there are only two items in each tuple, the return of array_column() will be an empty array, and thus in_array() will also always return false.

westonruter · 2024-01-09T22:34:01Z

@felixarntz

For now, I left a few comments on the production code changes. They mostly look good to me, although I think the code of some of those functions is extremely complex and could be simplified by separating responsibilities, and using smaller functions, or classes where applicable, for distinct processes. This is by no means a blocker for this PR, but I think it is critical to make this module/plugin long-term maintainable and more accessible to other contributors. [...] For the production code changes and tests, I don't feel strongly that they need to be separated, though I believe that it could help getting the production code changes merged a lot faster. Reviewing the tests takes notably more time.

Agreed. As mentioned above, I wanted to limit further refactoring until tests were written. The code changes not related to test coverage were primarily to fix blatant issues I uncovered while writing the tests. Sorry it grew so large. Since the review cycles have been pretty long, it's hard to keep the PRs very concise.

I think the PHPCS changes that affect the entire plugin make sense too, though I think that would be great to decouple as well, so that it can already be merged. FWIW, this probably shouldn't happen in the feature branch at all, as it likely will lead to more and more merge conflicts, the longer it takes for this feature branch to be merged. Could you open a PR with the PHPCS adjustments that don't affect the module directly against trunk?

Agreed. Done in #924. I've reverted the changes in this PR via 2181d88.

westonruter · 2024-01-10T00:36:08Z

Unit tests are failing due to Audit_Enqueued_Assets_Tests, which isn't related to the changes in this PR. ~~Two of the failures are fixed in trunk and~~ the other is fixed in #926 (which for some reason doesn't happen in trunk).

westonruter · 2024-01-10T00:43:35Z

The PHPStan failures are likewise not related to code changes here. They may be resolved by merging trunk into the feature branch, but I will wait to do that until #926 is merged.

@joemcgill

…om/WordPress/performance into add/ilo-tests * 'feature/image-loading-optimization' of https://github.com/WordPress/performance: (155 commits) Remove scheduler from globals since not yet used Fix or ignore eslint rules Remove modules/images/webp-uploads/fallback.js from ignorePatterns Run format-js on JS files Cherry pick fixes to JS linting from feature/image-loading-optimization Prevent sending header during test Manually fix remaining phpcs issues Run php-format Discontinue excluding all WordPress-Extra from tests so phpcbf can format Add @joemcgill to images and measurement focuses. Update codeowners, removing inactive contributors. Introduce perflab-plugin-management.js Load migration script in settings page Update function name Apply suggestions from code review Address slow query feedbacks Revise migration pointer un-dismissal to work for all admins Rename variable. improve code Require Node v20 ...

felixarntz

Thanks @westonruter, LGTM! I think this is good to merge once the PHPStan problems have been addressed (here or via the other PR).

Let's separately follow up on simplifying some of the logic with classes, and also file structure (see my comment below).

felixarntz · 2024-01-11T18:48:48Z

tests/modules/images/image-loading-optimization/storage/data-tests.php

+	 * @covers ::ilo_can_optimize_response
+	 * @dataProvider data_provider_test_ilo_can_optimize_response
+	 */
+	public function test_ilo_can_optimize_response( Closure $set_up, bool $expected ) {


I was just wondering why this function is in the storage/data.php file. It's only called in optimization.php, and the function name to me suggests it would fit better in there.

I then noticed storage/data.php contains several other functions like this, that to me don't seem storage related. Is that intentional? If so, can you clarify how you see the responsibilities between these two files?

Maybe there's a more intuitive way to break apart the functions here. To be fair, some of it may sort of automatically be addressed when using classes for some of those things as applicable.

Good catch! Function moved in ed305a9.

The other functions are generally storage related in that they relate to how fresh the stored data is, how to determine the slug for the data, manipulating the stored data, etc. But I agree this the functions in this file are much more of a grab bag compared to the functions in other files. I'll work to organize them better with the refactoring.

felixarntz · 2024-01-11T18:52:24Z

tests/modules/images/image-loading-optimization/storage/data-tests.php

+		$slug   = ilo_get_url_metrics_slug( array() );
+		$nonce1 = ilo_get_url_metrics_storage_nonce( $slug );
+		$this->assertMatchesRegularExpression( '/^[0-9a-f]{10}$/', $nonce1 );
+		$this->assertSame( 1, ilo_verify_url_metrics_storage_nonce( $nonce1, $slug ) );


Nit-pick: I find the return of 0, 1, or 2 an odd choice. I realize wp_verify_nonce() returns that, but it's extremely unintuitive, and I don't think many people know, except from reading the docs.

There's no value for us (and probably any other use-cases) to make a difference between when the nonce was generated, so I'd prefer to just return a boolean (i.e. cast the result to a bool). You aren't testing the 2 return value here either as far as I can tell (which makes sense IMO!), so that confirms it's useless to us :)

Yeah, sounds good. Done in d865bac.

Fix PHPStan errors in tests

… add/ilo-tests * 'trunk' of https://github.com/WordPress/performance: Re-run composer update with PHP 8.1 Run composer update Fix PHPStan errors in tests

modules/images/image-loading-optimization/class-ilo-html-tag-processor.php

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

mukeshpanchal27

Thank you, @westonruter, for the updates. I've left some minor suggestions, but they don't pose any blockers.

It would be nice to add @param annotations for the parameters used in the dataProvider for tests.

mukeshpanchal27 · 2024-01-12T05:13:38Z

tests/modules/images/image-loading-optimization/class-ilo-html-tag-processor-tests.php

+	 * @covers ::open_tags
+	 * @covers ::get_xpath
+	 *
+	 * @dataProvider data_provider_sample_documents


Nice if we add @param that used in dataProvider.

Yeah, somewhat. But it looks like most of the other tests using data providers don't have @param tags in the plugin. Not that this is a good reason.

tests/modules/images/image-loading-optimization/detection-tests.php

tests/modules/images/image-loading-optimization/optimization-tests.php

tests/modules/images/image-loading-optimization/storage/data-tests.php

tests/modules/images/image-loading-optimization/storage/lock-tests.php

tests/modules/images/image-loading-optimization/storage/post-type-tests.php

tests/modules/images/image-loading-optimization/storage/rest-api-tests.php

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

westonruter added no milestone PRs that do not have a defined milestone for release [Plugin] Optimization Detective Issues for the Optimization Detective plugin labels Dec 6, 2023

westonruter added 7 commits December 5, 2023 17:24

Add tests for detection.php and fix prefix on filter

5183678

Rename load tests to hooks tests

b1e6f9d

Remove needless reliance on ImagesTestCase

365bd43

Add tests for storage/lock.php

2190c28

Add tests for post type; improve validation of parsed URL metric

3b5a049

Add stubs for data tests

c0ef937

Add tests for REST API

e1a13d6

westonruter force-pushed the add/ilo-tests branch from 4906f55 to e1a13d6 Compare December 6, 2023 01:25

westonruter added the [Focus] Images label Dec 6, 2023

westonruter added 10 commits December 6, 2023 09:56

Discontinue excluding all of WordPress-Extra sniffs from tests and fi…

5c83545

…x issues

Add half of tests for storage/data.php functions

7d8afd8

Make ilo_unshift_url_metrics() easier to test

00b9ccb

Add test for ilo_unshift_url_metrics() and improve logic

c9d41c3

Add tests for functions to get sample size and breakpoint max widths

ea08eda

Add test for ilo_group_url_metrics_by_breakpoint

0f922f3

Add tests for ilo_get_lcp_elements_by_minimum_viewport_widths()

a6d6997

Add remainint test stubs

1d92756

Add tests for 2 optimization functions and improve comments

63136ab

Fix XPath for foreign elements and add tests for ILO_HTML_Tag_Processor

746f463

westonruter mentioned this pull request Dec 8, 2023

Rely on server-side breadcrumbs for detection and optimization #892

Merged

3 tasks

westonruter commented Dec 8, 2023

View reviewed changes

westonruter added 6 commits December 12, 2023 09:37

Add remaining tests for ILO_HTML_Tag_Processor

59fb175

Add test for ilo_needs_url_metric_for_breakpoint and fix stub

3e77988

Add two test cases for ilo_construct_preload_links and remove integri…

46080ef

…ty attr lifting

Add array shape typing for ilo_construct_preload_links() arg

e12b798

Add remaining tests for ilo_construct_preload_links()

0410678

Add tests for ilo_optimize_template_output_buffer()

1b16857

westonruter commented Dec 13, 2023

View reviewed changes

westonruter commented Jan 9, 2024

View reviewed changes

westonruter mentioned this pull request Jan 9, 2024

Update PHPCS formatting in tests #924

Merged

3 tasks

Revert test changes moved to #924

2181d88

Unrevert 2181d88 for server-timing-tests.php

930d4ed

westonruter mentioned this pull request Jan 10, 2024

Prevent sending Location header during test_perflab_aea_clean_aea_audit_action #926

Merged

3 tasks

westonruter requested a review from felixarntz January 10, 2024 00:43

westonruter added 5 commits January 9, 2024 16:50

Unrevert 2181d88 for audit-enqueued-assets-test.php

8dd3283

Reset SERVER global after each test

bf1bd17

Unset REQUEST_URI in tests that call go_to

54396ec

Fix PHPStan errors in tests

461ca6b

westonruter mentioned this pull request Jan 11, 2024

Fix PHPStan errors in tests #932

Merged

3 tasks

felixarntz approved these changes Jan 11, 2024

View reviewed changes

felixarntz mentioned this pull request Jan 11, 2024

Use class instances for common objects handled in image loading optimization logic #933

Closed

westonruter and others added 6 commits January 11, 2024 15:02

Run composer update

5c009ba

Re-run composer update with PHP 8.1

c5053f1

Update ilo_verify_url_metrics_storage_nonce() to return bool

d865bac

Move ilo_can_optimize_response() to optimization.php

ed305a9

Merge pull request #932 from WordPress/fix/phpstan-errors

e1c05c7

Fix PHPStan errors in tests

Merge branch 'trunk' of https://github.com/WordPress/performance into…

8874d07

… add/ilo-tests * 'trunk' of https://github.com/WordPress/performance: Re-run composer update with PHP 8.1 Run composer update Fix PHPStan errors in tests

mukeshpanchal27 reviewed Jan 12, 2024

View reviewed changes

modules/images/image-loading-optimization/class-ilo-html-tag-processor.php Show resolved Hide resolved

Add missing since tag

06ac5fe

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

mukeshpanchal27 approved these changes Jan 12, 2024

View reviewed changes

westonruter and others added 2 commits January 11, 2024 22:22

Add line break before dataProvider tag

4bb2ca8

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

Add additional line breaks in phpdoc

d62d710

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

westonruter merged commit 6433bc1 into feature/image-loading-optimization Jan 12, 2024

westonruter deleted the add/ilo-tests branch January 12, 2024 06:30

Comments

Conversation

westonruter commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Relevant technical choices

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter commented Jan 9, 2024

Uh oh!

westonruter commented Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonruter commented Jan 10, 2024

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

westonruter commented Dec 6, 2023 •

edited

Loading

westonruter commented Jan 10, 2024 •

edited

Loading