Joe Swatosh wrote on Mon, Mar 19, 2012 at 17:27:13 -0700:
> On Mon, Mar 19, 2012 at 9:09 AM, Hyrum K Wright
> <hyrum.wri...@wandisco.com> wrote:
> > On Tue, Mar 13, 2012 at 9:52 AM, Joe Swatosh <joe.swat...@gmail.com> wrote:
> >> On Mon, Mar 12, 2012 at 6:18 AM, Hyrum K Wright
> >> <hyrum.wri...@wandisco.com> wrote:
> >>> On Mon, Mar 12, 2012 at 12:11 AM, Joe Swatosh <joe.swat...@gmail.com>
> >>> wrote:
> >>>> On Fri, Mar 9, 2012 at 10:39 PM, Joe Swatosh <joe.swat...@gmail.com>
> >>>> wrote:
> >>>>> On Fri, Mar 9, 2012 at 9:46 PM, Joe Swatosh <joe.swat...@gmail.com>
> >>>>> wrote:
> >>>>>> On Fri, Mar 9, 2012 at 2:44 AM, Philip Martin
> >>>>>> <philip.mar...@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?
>
Haven't been following the problem, but from the peanut gallery that
doesn't sound right either.
> 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.
Good call, thanks.
