Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#35987 closed enhancement (fixed)

Unit test: wp_ext2type

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

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 10 years ago.
Patch, tests for wp_ext2type
functions_35987.diff (2.9 KB ) - added by borgesbruno 10 years ago.
Refactoring functions.php
test_functions_35987.diff (1.5 KB ) - added by borgesbruno 10 years ago.
35987.2.diff (4.4 KB ) - added by borgesbruno 10 years ago.
35987.3.diff (4.5 KB ) - added by borgesbruno 10 years ago.

Download all attachments as: .zip

Change History (17)

@borgesbruno
10 years ago

Patch, tests for wp_ext2type

#1 @borgesbruno
10 years ago

  • Keywords has-patch has-unit-tests added

#2 @DrewAPicture
10 years ago

  • Component GeneralMedia

#3 @boonebgorges
10 years ago

  • Keywords needs-patch added; has-patch has-unit-tests removed
  • Milestone Awaiting ReviewFuture 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
10 years ago

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

@borgesbruno
10 years ago

Refactoring functions.php

#5 @borgesbruno
10 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
10 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 10 years ago by johnbillion (previous) (diff)

@borgesbruno
10 years ago

#7 @borgesbruno
10 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
10 years ago

  • Keywords 4.6-early added
  • Owner set to boonebgorges
  • Status newassigned

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
10 years ago

#9 @borgesbruno
10 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 increase 1 second and half with almost two hundred assertions, running in an isolate mode.

Version 0, edited 10 years ago by borgesbruno (next)

#10 @boonebgorges
10 years ago

  • Keywords 4.6-early removed
  • Milestone Future Release4.6

#11 @boonebgorges
10 years ago

  • Resolutionfixed
  • Status assignedclosed

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
10 years ago

#36294 was marked as a duplicate.

Note: See TracTickets for help on using tickets.