Make WordPress Core

Opened 11 months ago

Closed 8 months ago

#56203 closed task (blessed) (fixed)

Reduce code duplication in AJAX attachment handling tests

Reported by: martinkrcho's profile martin.krcho Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.1 Priority: normal
Severity: minor Version: 6.1
Component: Media Keywords: has-patch
Focuses: Cc:

Description

The tests for AJAX attachment handling contain duplicated code to preform the following tasks.

a) create a new admin user and set them as the current user
https://github.com/WordPress/wordpress-develop/blob/6.0.0/tests/phpunit/tests/ajax/Attachments.php#L17-L27

b) upload selected file and create an attachment
https://github.com/WordPress/wordpress-develop/blob/6.0.0/tests/phpunit/tests/ajax/Attachments.php#L29-L33

It would be great to replace the duplicated code with some helper functions. It would make creating more tests simpler.

Attachments (1)

Reduce_duplication_in_attachments_AJAX_tests.patch (2.6 KB) - added by martin.krcho 11 months ago.
Patch reducing the duplication in the AJAX attachments handler tests

Download all attachments as: .zip

Change History (11)

#1 @martin.krcho
11 months ago

  • Keywords needs-patch added

@martin.krcho
11 months ago

Patch reducing the duplication in the AJAX attachments handler tests

#2 @SergeyBiryukov
11 months ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 6.1

#3 @SergeyBiryukov
11 months ago

In 53701:

Tests: Use a consistent way of setting the Administrator role in Ajax tests.

This removes some duplicate code in favor of calling the WP_Ajax_UnitTestCase::_setRole() method created specifically for this purpose and used in other tests.

Follow-up to [500/tests], [37288].

Props martin.krcho.
See #56203.

#4 @SergeyBiryukov
11 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

Hi there, thanks for the ticket and the patch! The first part is now committed.

The attachments part looks good too, but it appears that the same pattern is also used in other tests, not only Ajax. So I think this can be a general helper method in WP_UnitTestCase_Base, next to the ::_make_attachment() method. That way it can also be reused in other tests. The name would probably be _upload_file_and_make_attachment() for consistency.

#5 @martin.krcho
11 months ago

Thanks for committing the first part, @SergeyBiryukov.

I created a new ticket for the helper method handling file upload and attachment creation. Patch will be ready soon.

#6 follow-up: @martin.krcho
9 months ago

What's next for this ticket, @SergeyBiryukov? Can we close it since the changes are already committed? There is also a follow-up ticket (56248) that's ready for review.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


9 months ago

#8 @JeffPaul
9 months ago

  • Type changed from enhancement to task (blessed)

As discussed ahead of today's scheduled 6.1 Beta 1 release party (see the Slack link above), it was deemed that this ticket was now a test-only ticket and could be converted to a task for additional follow up in the remaining 6.1 cycle.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


9 months ago

#10 in reply to: ↑ 6 @SergeyBiryukov
8 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

Replying to martin.krcho:

Can we close it since the changes are already committed? There is also a follow-up ticket (#56248) that's ready for review.

Ah, I was going to circle back here for comment:4, but I see there is now a separate ticket for that, so let's continue there. This one can be closed. Thanks!

Note: See TracTickets for help on using tickets.