Re: Ruby bindings failing on buildbots

On Mon, Mar 12, 2012 at 12:11 AM, Joe Swatosh <joe.swatosh_at_gmail.com> wrote:
> On Fri, Mar 9, 2012 at 10:39 PM, Joe Swatosh <joe.swatosh_at_gmail.com> wrote:
>> On Fri, Mar 9, 2012 at 9:46 PM, Joe Swatosh <joe.swatosh_at_gmail.com> wrote:
>>> On Fri, Mar 9, 2012 at 2:44 AM, Philip Martin
>>> <philip.martin_at_wandisco.com> wrote:
>>>> The Ruby bindings are failing during check-swig-rb on the buildbots for
>>>> trunk.  This has been happening since r1293375, which changed the way
>>>> property diffs are reported during replay.  As far as I can tell this is
>>>> a problem with the bindings rather than the core Subversion code.
>>>>
>>>> The test deletes and commits, along with other changes, a property 'AAA'
>>>> with value 'BBB' from file '/diff1.txt'.  I've added some print
>>>> statements to the Ruby and I see this for the test at r1293374:
>>>>
>>>> test_diff(SvnInfoTest): ChangedEditor.change_file_prop:diff1.txt:AAA:nil
>>>> ChangedEditor.close_file:diff1.txt:update
>>>> ChangedEditor.close_file:diff2.txt:nil
>>>> ChangedEditor.change_file_prop:nil:XXX:YYY
>>>> ChangedEditor.close_file:nil:update
>>>> ChangedEditor.close_file:nil:nil
>>>> ChangedEditor.close_edit:diff1.txtdiff2.txt
>>>> get_diff_recurse::false:false
>>>> get_diff_recurse:/diff1.txt:true:true   # it's a file with prop_mod
>>>> get_prop_diff:/diff1.txt:AAABBB
>>>> Property.diffs2::AAABBB
>>>> get_diff_recurse:/diff2.txt:true:false
>>>> get_diff_recurse:/diff4.txt:true:true
>>>> get_prop_diff:/diff4.txt:
>>>> Property.diffs2:XXXYYY:
>>>> get_diff_recurse:/diff5.txt:true:false
>>>> .
>>>>
>>>> At r1293375 I see this:
>>>>
>>>> test_diff(SvnInfoTest): ChangedEditor.change_file_prop:diff1.txt:AAA:nil
>>>> ChangedEditor.close_file:diff1.txt:update
>>>> ChangedEditor.close_file:diff2.txt:nil
>>>> ChangedEditor.change_file_prop:nil:XXX:YYY
>>>> ChangedEditor.close_file:nil:update
>>>> ChangedEditor.close_file:nil:nil
>>>> ChangedEditor.close_edit:diff1.txtdiff2.txt
>>>> get_diff_recurse::false:false
>>>> get_diff_recurse:/diff1.txt:true:false   # it's a file, no prop_mod
>>>> get_diff_recurse:/diff2.txt:true:false
>>>> get_diff_recurse:/diff4.txt:true:true
>>>> get_prop_diff:/diff4.txt:
>>>> Property.diffs2:XXXYYY:
>>>> get_diff_recurse:/diff5.txt:true:false
>>>> F
>>>>
>>>> So in both cases the property delete is getting into the Ruby bindings
>>>> at ChangedEditor.change_file_prop, but gets lost by the time
>>>> get_diff_recurse is reached.
>>>>
>>>> I don't know enough Ruby to fix this.
>>>>
>>>> --
>>>> uberSVN: Apache Subversion Made Easy
>>>> http://www.uberSVN.com
>>>
>>> Thanks for saying something. I'll try to look into it this weekend.
>>>
>>> --
>>> Joe
>>
>> Anybody else gets to it first, please feel free to fix. I've got to
>> figure out when gen-make.py broke for me .  :-(
>>
>> --
>> Joe
>
> Upgrading Python fixed my build, but I'm still way behind head so I
> don't want to commit this myself...
>
> Thanks,
> --
> Joe
>
> ****************
>
> Restore failing Ruby bindings tests failing since r1293375.
>
> * subversion/bindings/swig/ruby/test/test_info.rb
>  (test_diff): Remove assertions testing implementation details that
> have changed.
>
> ****************
>
>
> Index: subversion/bindings/swig/ruby/test/test_info.rb
> ===================================================================
> --- subversion/bindings/swig/ruby/test/test_info.rb     (revision 1294254)
> +++ subversion/bindings/swig/ruby/test/test_info.rb     (working copy)
> @@ -217,7 +217,6 @@
>       assert_equal([file1, file2, file4].sort, keys[0..-2])
>       assert_match(/\A#{file5}/, file5_key)
>       assert(info.diffs[file1].has_key?(:modified))
> -      assert(info.diffs[file1].has_key?(:property_changed))
>       assert(info.diffs[file2].has_key?(:modified))
>       assert(info.diffs[file4].has_key?(:added))
>       assert(info.diffs[file4].has_key?(:property_changed))
> @@ -230,8 +229,6 @@
>       assert_equal(0, info.diffs[file4][:added].deleted_line)
>       assert_equal(0, info.diffs[file5_key][:copied].added_line)
>       assert_equal(0, info.diffs[file5_key][:copied].deleted_line)
> -      assert_equal("Name: #{file1_prop_key}\n   - #{file1_prop_value}\n",
> -                   info.diffs[file1][:property_changed].body)
>       assert_equal("Name: #{file4_prop_key}\n   + #{file4_prop_value}\n",
>                    info.diffs[file4][:property_changed].body)
>       assert_equal(commit_info.revision, info.revision)

That would certainly fix the test failures, in that they wouldn't be detected.

Are you implying the current (without this patch) ruby tests are
testing implementation details, as well as results, and that's the
reason this change is needed?

-Hyrum

-- 
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/