WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#35987 closed enhancement (fixed)

Unit test: wp_ext2type

Reported by: borgesbruno Owned by: boonebgorges
Milestone: 4.6 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-unit-tests has-patch
Focuses: Cc:
PR Number:

Description

I wrote tests for the method wp_ext2type to improve the code coverage, there weren't no tests for this method.

Attachments (5)

35987.diff (770 bytes) - added by borgesbruno 4 years ago.
Patch, tests for wp_ext2type
functions_35987.diff (2.9 KB) - added by borgesbruno 4 years ago.
Refactoring functions.php
test_functions_35987.diff (1.5 KB) - added by borgesbruno 4 years ago.
35987.2.diff (4.4 KB) - added by borgesbruno 4 years ago.
35987.3.diff (4.5 KB) - added by borgesbruno 4 years ago.

Download all attachments as: .zip

Change History (17)

@borgesbruno
4 years ago

Patch, tests for wp_ext2type

#1 @borgesbruno
4 years ago

  • Keywords has-patch has-unit-tests added

#2 @DrewAPicture
4 years ago

  • Component changed from General to Media

#3 @boonebgorges
4 years ago

  • Keywords needs-patch added; has-patch has-unit-tests removed
  • Milestone changed from Awaiting Review to Future Release

Thanks for the patch.

Your tests perform spot-checks only. But I'm thinking that this is a case where we may want to refactor slightly to allow for complete test coverage. See, eg, wp_get_mime_types(), which is just a wrapper for a filtered array. How about introducing wp_get_ext_types(), moving the array there, and then using it as a dataProvider? We can then also have targeted tests for the peculiar behavior of in_array(), such as the 'JPG' =>

#4 @borgesbruno
4 years ago

Actually, this is a such great idea @boonebgorges, I going to do that.
Tks for the review!

@borgesbruno
4 years ago

Refactoring functions.php

#5 @borgesbruno
4 years ago

  • Keywords has-patch has-unit-tests added; needs-patch removed

@boonebgorges I made the changes that you said, I didn't test each extension in the array provider, because using just one as sample is better for performance and covers the whole logic.
Let me know if I get your point =)

#6 @johnbillion
4 years ago

  • Keywords needs-patch added; has-patch removed

@borgesbruno Thanks for the updated patch. A few quick bits of feedback:

  • The ext2type filter has gone missing.
  • Can you combine the functional change and the unit tests into one patch? This makes testing easier.
  • When you upload a new patch, you can use the same file name (35987.diff) each time and Trac will automatically rename it for you.
Last edited 4 years ago by johnbillion (previous) (diff)

@borgesbruno
4 years ago

#7 @borgesbruno
4 years ago

  • Keywords has-patch added; needs-patch removed

Hi again @johnbillion , tks for the great tips, Its done, following all your requests ;-)
This is the file = 35987.2.diff​

#8 @boonebgorges
4 years ago

  • Keywords 4.6-early added
  • Owner set to boonebgorges
  • Status changed from new to assigned

Thanks, @borgesbruno! I think we should go ahead and run tests against every single extension - for such a small number, the performance implications will probably be too small to measure.

@borgesbruno
4 years ago

#9 @borgesbruno
4 years ago

Tks for the feedback @boonebgorges, it's done!
I made the tests for each extension and just for curiosity I also made a benchmark:

The patch 2
Time: 12.68 seconds, Memory: 122.25Mb
OK (1 test, 19 assertions)

The patch 3
Time: 14.19 seconds, Memory: 122.25Mb
OK (1 test, 197 assertions)

As you can see you were right, just increased one second and half with almost two hundred assertions, running in an isolate mode.

Last edited 4 years ago by borgesbruno (previous) (diff)

#10 @boonebgorges
4 years ago

  • Keywords 4.6-early removed
  • Milestone changed from Future Release to 4.6

#11 @boonebgorges
4 years ago

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

In 37189:

Improve testability and coverage of wp_ext2type().

  • Following pattern of wp_get_mime_types(), introduce wp_get_ext_types() function. New function returns a filtered list of file types with their extensions.
  • Use this function in new tests for wp_ext2type().

Props borgesbruno.
Fixes #35987.

#12 @flixos90
4 years ago

#36294 was marked as a duplicate.

Note: See TracTickets for help on using tickets.