Re: Ruby bindings failing on buildbots

On Mon, Mar 19, 2012 at 9:09 AM, Hyrum K Wright
<hyrum.wright_at_wandisco.com> wrote:
> On Tue, Mar 13, 2012 at 9:52 AM, Joe Swatosh <joe.swatosh_at_gmail.com> wrote:
>> On Mon, Mar 12, 2012 at 6:18 AM, Hyrum K Wright
>> <hyrum.wright_at_wandisco.com> wrote:
>>> 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:
>>
>>>>
>>>> ****************
>>>>
>>>> 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
>>>
>>>
>>
>> Yup that is exactly what I'm implying. You may recall during wc-ng
>> development that there were many failing Ruby bindings tests. There
>> were three broad categories of failures: binding or binding test
>> errors, unintentional changes to how the wc library worked, and tests
>> of wc implementation among the bindings tests. My recollection of that
>> time was that the problems were pretty evenly distributed into those
>> categories. When I asked him about it, my memory of what kou said was
>> that subversion implementation needed testing.
>>
>> WRT this patch, when I sent it, I thought this was an implementation
>> test, but on reflection I am not so sure. I will look into it again
>> when the weekend comes (earliest that my schedule allows). If anyone
>> can resolve this correctly sooner, I encourage them to do so.
>
> I committed the patch in r1302524.  Feel free to update it as you feel
> necessary.
>
> -Hyrum
>

I understand needing to get the buildbots green again, however I think
that commit should be reverted. If the problem is in the bindings, I
think this is the patch that is needed:

[[[

Index: subversion/bindings/swig/ruby/svn/info.rb
===================================================================
--- subversion/bindings/swig/ruby/svn/info.rb (revision 1302724)
+++ subversion/bindings/swig/ruby/svn/info.rb (working copy)
@@ -156,7 +156,7 @@
         try_diff(node, base_root, path, base_path)
       end

- if node.prop_mod? and !node.delete?
+ unless node.delete?
         get_prop_diff(node, base_root, path, base_path)
       end

]]]

At least this will get the original test passing again. I guess that
deleting a property isn't considered a prop_mod on the node anymore?

I'm only 99% on this patch, as I need more time to make sure it
doesn't cause reporting of empty differences in other situations.

--
Joe
PS I have different patch that provides an implementation of
Dir.mktmpdir if one doesn't exist (like for Ruby 1.8.6), but it is
just a copy of the implementation from Ruby 1.8.7. There is no copy
info in the original file and I want to double check how to properly
attribute it.  Thanks.