Re: Recent changes broke some tests

Great, thanks Shad.  When it comes to testing, I mainly focus on the
helpers and models so that core integrity is maintained.  I do try to test
the trickier controllers, but frankly controllers shouldn't get too
complicated or we wind up with a bunch of functionality that we have to
reproduce in our REST code.

There are definitely cases where we introduced a useful helper but didn't
propagate its usage widely - that's mainly a function of having too many
exigent issues.. I'm really happy to see you cleaning these up.

-Bharat


On Thu, Jan 17, 2013 at 1:03 PM, Shad Laws <shad-xpYdmXCiSuZWk0Htik3J/[email protected]> wrote:

> Hey Bharat,
>
> You're quite welcome, and thanks also for your note!  It's fun to be
> involved.
>
> Re: the issue with legal_file, I too had found the same problem.  Bummer.
>  Back when I made the mod I was a little lax too, and also started
> tightening up and running (and adding to) unit tests.  A few days ago I
> filed a ticket (with reference to the original ticket) and submitted a pull
> that's awaiting approval:
> http://sourceforge.net/apps/trac/gallery/ticket/1944
> https://github.com/gallery/gallery3/pull/90
>
> The pull also addresses another robustness problem (#1943) and extends
> functionality a bit (#1945 - there are two sets of functions in there that
> are mostly-but-not-totally symmetric with regard to their functionality,
> and this changes that).  The part fixing #1944 is more-or-less identical to
> what you did, so I'll need to redo that pull.  In that redo, I'll also add
> a couple more unit tests as you suggested.
>
> On the note of unit tests, I'd also noticed recently that there doesn't
> seem to be any that really test movies or corrupted photos.  I'm happy to
> see you beat me to the punch on the first item :-).  The latter I'd
> stumbled across testing the changes to missing_photo we discussed... and
> finding out how incredibly hard it is to get Gallery to actually use it
> (for some reason it's currently only used if the rebuild does *not* crash
> but *does* produce an empty file).  I'm working on changing that...
>
> It really isn't the culprit here, but I'd also been recently looking at
> photo::get_file_metadata and realizing it needs some help.  First problem
> is that, although it's a nicely-placed helper function, most places in the
> core code don't actually use it (they just call getimagesize on their own).
>  In addition to duplicating code, this makes it pretty hard to extend to
> other file types.  I'm working on changing that, too...
>
> Thanks again!
>
> Take care,
> Shad
>
>
>
>
> On 17 January 2013 21:07, Bharat Mediratta <[email protected]> wrote:
>
>> (cc gallery-devel to get more eyes involved)
>>
>> Hey Shad.  First of all, let me just say a big thanks for your recent
>> code contributions to G3.  We've recently had a dearth of competent
>> engineers who are able to make core contributions and yours have been high
>> quality.
>>
>> In reviewing the recent pull request:
>> https://github.com/gallery/gallery3/pull/91/files
>>
>> I realized that this change really needs some unit tests to verify that
>> we're not breaking existing behavior.  I realize with previous pulls I've
>> been lax about making sure that our tests are up to snuff and passing.
>>
>> In running the tests, I found some failures with the recent changes.
>>  Specifically in
>> https://github.com/gallery/gallery3/commit/94b26e506c339f50b8d094057bffc1877a79afa9the change to photo::get_file_metadata causes
>> Item_Model_Test::unsafe_data_file_replacement_test to fail because the new
>> legal_file::get_photos_by_extension doesn't know what to do with an unknown
>> extension type.  The test injects "php" and expects that to fail since it's
>> an unexpected extension type.
>>
>> I tweaked the legal_file code to be more robust
>> in d04080c7be7c8a06bd81a9747943600812339f40 - but before we proceed I think
>> it's probably worthwhile to add some new tests to Legal_File_Test to cover
>> edge cases in the new functions you added.  Can you do that?
>>
>> In the meantime I'm to run all the other tests and make sure we're clean
>> before going back to the pull requests.
>>
>> -Bharat
>>
>
>

------------------------------------------------------------------------------
Master HTML5, CSS3, ASP.NET, MVC, AJAX, Knockout.js, Web API and
much more. Get web development skills now with LearnDevNow -
350+ hours of step-by-step video tutorials by Microsoft MVPs and experts.
SALE $99.99 this month only -- learn more at:
http://p.sf.net/sfu/learnmore_122812

__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]