Re: Status of TODO-1.6

Hi,

In <495A9FB2.8010304_at_mail.utexas.edu>
  "Re: Status of TODO-1.6" on Tue, 30 Dec 2008 16:24:50 -0600,
  "Hyrum K. Wright" <hyrum_wright_at_mail.utexas.edu> wrote:

> Kou,
> As Peter mentioned, whatever attachment you included was wiped by the new tigris
> Discussion Services. Could you resend with the attachment included inline?

OK.
Here is the attachment:

Index: subversion/bindings/swig/ruby/test/test_client.rb
===================================================================
--- subversion/bindings/swig/ruby/test/test_client.rb (revision 34960)
+++ subversion/bindings/swig/ruby/test/test_client.rb (working copy)
@@ -920,7 +920,140 @@
     assert_equal([], statuses)
   end
 
- def assert_merge
+ class AssertMergeData
+ attr_accessor :log, :file, :src, :trunk, :branch
+ attr_accessor :branch_relative_uri, :branch_uri
+ attr_accessor :trunk_path, :trunk_path_uri
+ attr_accessor :branch_path, :branch_path_relative_uri, :branch_path_uri
+ attr_accessor :rev1, :rev2, :rev3, :rev4, :rev5, :rev6
+
+ def initialize(attributes)
+ attributes.each do |name, value|
+ send("#{name}=", value)
+ end
+ end
+ end
+
+ def assert_merge_before_merge(ctx, data, &merge)
+ ctx.mkdir(data.trunk, data.branch)
+ File.open(data.trunk_path, "w") {}
+ File.open(data.branch_path, "w") {}
+ ctx.add(data.trunk_path)
+ ctx.add(data.branch_path)
+ data.rev1 = ctx.commit(@wc_path).revision
+
+ File.open(data.branch_path, "w") {|f| f.print(data.src)}
+ data.rev2 = ctx.commit(@wc_path).revision
+
+ merged_entries = []
+ ctx.log_merged(data.trunk, nil, data.branch_uri, nil) do |entry|
+ merged_entries << entry
+ end
+ assert_equal_log_entries([], merged_entries)
+ assert_nil(ctx.merged(data.trunk))
+ end
+
+ def assert_merge_merge_between_rev1_and_rev2(ctx, data, &merge)
+ merged_entries = []
+ merge.call(ctx, data.branch, data.rev1, data.rev2, data.trunk)
+ ctx.log_merged(data.trunk, nil, data.branch_uri, nil) do |entry|
+ merged_entries << entry
+ end
+ assert_equal_log_entries([
+ [
+ {data.branch_path_relative_uri => ["M", nil, -1]},
+ data.rev2,
+ {
+ "svn:author" => @author,
+ "svn:log" => data.log,
+ },
+ false,
+ ]
+ ],
+ merged_entries)
+ mergeinfo = ctx.merged(data.trunk)
+ assert_not_nil(mergeinfo)
+ assert_equal([data.branch_uri], mergeinfo.keys)
+ ranges = mergeinfo[data.branch_uri].collect {|range| range.to_a}
+ assert_equal([[data.rev1, data.rev2, true]], ranges)
+
+ data.rev3 = ctx.commit(@wc_path).revision
+ assert_equal(normalize_line_break(data.src),
+ ctx.cat(data.trunk_path, data.rev3))
+ end
+
+ def assert_merge_remove_path_and_merge_into_trunk(ctx, data, &merge)
+ ctx.rm(data.branch_path)
+ data.rev4 = ctx.commit(@wc_path).revision
+
+ merge.call(ctx, data.branch, data.rev3, data.rev4, data.trunk)
+ assert(!File.exist?(data.trunk_path))
+
+ merged_entries = []
+ ctx.log_merged(data.trunk, data.rev4, data.branch_uri, data.rev4) do |entry|
+ merged_entries << entry
+ end
+ assert_equal_log_entries([
+ [
+ {data.branch_path_relative_uri => ["D", nil, -1]},
+ data.rev4,
+ {
+ "svn:author" => @author,
+ "svn:log" => data.log,
+ },
+ false,
+ ]
+ ] * 2, merged_entries)
+
+ ctx.propdel("svn:mergeinfo", data.trunk)
+ merged_entries = []
+ ctx.log_merged(data.trunk, data.rev4, data.branch_uri, data.rev4) do |entry|
+ merged_entries << entry
+ end
+ assert_equal_log_entries([], merged_entries)
+ end
+
+ def assert_merge_revert_merged_and_cause_conflict_and_resolve(ctx, data,
+ &merge)
+ ctx.revert(data.trunk)
+ ctx.revert(data.trunk_path)
+ File.open(data.trunk_path, "a") {|f| f.print(data.src)}
+ merge.call(ctx, data.branch, data.rev3, data.rev4, data.trunk)
+ ctx.resolved(data.trunk, false)
+ data.rev5 = ctx.commit(@wc_path).revision
+ assert(File.exist?(data.trunk_path))
+ end
+
+ def assert_merge_remerge_force(ctx, data, &merge)
+ merge.call(ctx, data.branch, data.rev3, data.rev4, data.trunk,
+ nil, false, true)
+ statuses = []
+ ctx.status(data.trunk) do |_, status|
+ statuses << status
+ end
+ assert_equal([[Svn::Wc::STATUS_NORMAL, Svn::Wc::STATUS_MODIFIED]],
+ statuses.collect do |status|
+ [status.text_status, status.prop_status]
+ end)
+ end
+
+ def assert_merge_remove_merge_info(ctx, data, &merge)
+ ctx.propdel("svn:mergeinfo", data.trunk)
+ data.rev6 = ctx.commit(@wc_path).revision
+ end
+
+ def assert_merge_remerge_force_again_dry_run(ctx, data, &merge)
+ yield(ctx, data.branch, data.rev3, data.rev4, data.trunk,
+ nil, false, true, true)
+ assert_not_changed(ctx, data.trunk)
+ end
+
+ def assert_merge_remerge_force_again(ctx, data, &merge)
+ yield(ctx, data.branch, data.rev3, data.rev4, data.trunk, nil, false, true)
+ assert_changed(ctx, data.trunk)
+ end
+
+ def assert_merge(&merge)
     log = "sample log"
     file = "sample.txt"
     src = "sample\n"
@@ -934,105 +1067,32 @@
     branch_path_relative_uri = "#{branch_relative_uri}/#{file}"
     branch_path_uri = "#{@repos_uri}#{branch_path_relative_uri}"
 
+ attributes = {
+ :log => log,
+ :file => file,
+ :src => src,
+ :trunk => trunk,
+ :branch => branch,
+ :branch_relative_uri => branch_relative_uri,
+ :branch_uri => branch_uri,
+ :trunk_path => trunk_path,
+ :trunk_path_uri => trunk_path_uri,
+ :branch_path => branch_path,
+ :branch_path_relative_uri => branch_path_relative_uri,
+ :branch_path_uri => branch_path_uri,
+ }
     make_context(log) do |ctx|
- ctx.mkdir(trunk, branch)
- File.open(trunk_path, "w") {}
- File.open(branch_path, "w") {}
- ctx.add(trunk_path)
- ctx.add(branch_path)
- rev1 = ctx.commit(@wc_path).revision
+ data = AssertMergeData.new(attributes)
 
- File.open(branch_path, "w") {|f| f.print(src)}
- rev2 = ctx.commit(@wc_path).revision
-
- merged_entries = []
- ctx.log_merged(trunk, nil, branch_uri, nil) do |entry|
- merged_entries << entry
- end
- assert_equal_log_entries([], merged_entries)
- assert_nil(ctx.merged(trunk))
-
- merged_entries = []
- yield(ctx, branch, rev1, rev2, trunk)
- ctx.log_merged(trunk, nil, branch_uri, nil) do |entry|
- merged_entries << entry
- end
- assert_equal_log_entries([
- [
- {branch_path_relative_uri => ["M", nil, -1]},
- rev2,
- {
- "svn:author" => @author,
- "svn:log" => log,
- },
- false,
- ]
- ],
- merged_entries)
- mergeinfo = ctx.merged(trunk)
- assert_not_nil(mergeinfo)
- assert_equal([branch_uri], mergeinfo.keys)
- ranges = mergeinfo[branch_uri].collect {|range| range.to_a}
- assert_equal([[1, 2, true]], ranges)
-
- rev3 = ctx.commit(@wc_path).revision
-
- assert_equal(normalize_line_break(src), ctx.cat(trunk_path, rev3))
-
- ctx.rm(branch_path)
- rev4 = ctx.commit(@wc_path).revision
-
- yield(ctx, branch, rev3, rev4, trunk)
- assert(!File.exist?(trunk_path))
-
- merged_entries = []
- ctx.log_merged(trunk, rev4, branch_uri, rev4) do |entry|
- merged_entries << entry
- end
- assert_equal_log_entries([
- [
- {branch_path_relative_uri => ["D", nil, -1]},
- rev4,
- {
- "svn:author" => @author,
- "svn:log" => log,
- },
- false,
- ]
- ] * 2, merged_entries)
-
- ctx.propdel("svn:mergeinfo", trunk)
- merged_entries = []
- ctx.log_merged(trunk, rev4, branch_uri, rev4) do |entry|
- merged_entries << entry
- end
- assert_equal_log_entries([], merged_entries)
-
- ctx.revert(trunk)
- ctx.revert(trunk_path)
- File.open(trunk_path, "a") {|f| f.print(src)}
- yield(ctx, branch, rev3, rev4, trunk)
- ctx.resolved(trunk,false)
- rev5 = ctx.commit(@wc_path).revision
- assert(File.exist?(trunk_path))
-
- yield(ctx, branch, rev3, rev4, trunk, nil, false, true)
- statuses = []
- ctx.status(trunk) do |_, status|
- statuses << status
- end
- assert_equal(1, statuses.size, "Only one entry should have changed")
- assert_equal(Svn::Wc::STATUS_NORMAL, statuses.first.text_status, "No changes to file content expected")
- assert_equal(Svn::Wc::STATUS_MODIFIED, statuses.first.prop_status, "merge info changes")
-
- ctx.propdel("svn:mergeinfo", trunk)
- rev6 = ctx.commit(@wc_path).revision
-
- yield(ctx, branch, rev3, rev4, trunk, nil, false, true, true)
- assert_not_changed(ctx, trunk)
-
- yield(ctx, branch, rev3, rev4, trunk, nil, false, true)
- assert_changed(ctx, trunk)
+ assert_merge_before_merge(ctx, data, &merge)
+ assert_merge_merge_between_rev1_and_rev2(ctx, data, &merge)
+ assert_merge_remove_path_and_merge_into_trunk(ctx, data, &merge)
+ assert_merge_revert_merged_and_cause_conflict_and_resolve(ctx, data,
+ &merge)
+ assert_merge_remerge_force(ctx, data, &merge)
+ assert_merge_remove_merge_info(ctx, data, &merge)
+ assert_merge_remerge_force_again_dry_run(ctx, data, &merge)
+ assert_merge_remerge_force_again(ctx, data, &merge)
     end
   end
 

Thanks,

--
kou
> Kouhei Sutou wrote:
> > Hi,
> > 
> > In <ae6cb1100812271118u36c2ae9aj7b773b770bff303b_at_mail.gmail.com>
> >   "Re: Status of TODO-1.6" on Sat, 27 Dec 2008 11:18:49 -0800,
> >   Joe Swatosh <joe.swatosh_at_gmail.com> wrote:
> > 
> >> Hi kou,
> >>
> >> On Fri, Dec 26, 2008 at 11:12 AM, Hyrum K. Wright
> >> <hyrum_wright_at_mail.utexas.edu> wrote:
> >>> Kouhei Sutou wrote:
> >>>> Hi,
> >>>>
> >>>> In <495271AD.8020401_at_mail.utexas.edu>
> >>>>   "Status of TODO-1.6" on Wed, 24 Dec 2008 11:30:21 -0600,
> >>>>   "Hyrum K. Wright" <hyrum_wright_at_mail.utexas.edu> wrote:
> >>>>
> >>>>>  * Test failures in Ruby bindings.
> >>>>>
> >>>>> These just look like they are the result of wrong expectations due to tree
> >>>>> conflicts, but my understanding of ruby and the swig-rb bindings is so
> >>>>> rudimentary as to remove all confidence in my ability to track it down.  Kou,
> >>>>> Joe, any suggestions?
> >>>> The current test suits were all passed when they were
> >>>> created. It means the current Subversion behavior is changed
> >>>> since the time.
> >>>>
> >>>> I don't know about tree conflicts. If the current actual
> >>>> values are expected result, we should change the current
> >>>> expected values. But I can't decide it...
> >>> And I don't know enough about ruby and the test suite to determine exactly what
> >>> the offending tests are testing.  Either somebody with knowledge of both should
> >>> comment, or perhaps you can give us an overview of what the tests are doing, and
> >>> one of the tree conflicts people can comment on whether or not the new failures
> >>> are expected.
> >>>
> >> I find myself getting lost in assert_merge() too.  Could you either
> >> add some messages to the assertions or perhaps split assert_merge into
> >> some smaller named assertions to make it more clear?
> > 
> > OK. What about the attached patch?
> > 
> >> I think its tough to test the bindings, as we have to assert
> >> everything through side-effects, and some of the assertions seem like
> >> they are testing merge itself instead of the _binding_ to the merge
> >> function.  (Does that make sense?)
> > 
> > I think that the bindings can do all things that can be done
> > with the Subversion C library. It's the reason why the Ruby
> > bindings' test scenario is written based on the original
> > Subversion feature's work. (e.g. merging)
> > 
> > We can write the Ruby bindings' tests more roughly like
> > other bindings. If we do so, we can maintain the Ruby
> > bindings' tests more easily because the Ruby bindings' tests
> > have less failures than now in the feature Subversion
> > development. But I think that it may miss some bugs caused
> > by the Subversion C library's changes. I think we have tests
> > for detecting those bugs. (I remember that there were some
> > SEGV bugs detected by the Ruby bindings' tests.)
> > 
> > We have a tradeoff between easy maintenance and bug
> > detectability. The latter is more important for me. (Because
> > the current Ruby bindings' tests are tough.) But if the
> > Subversion project or Joe selects the former, I'll follow
> > the selection. The Ruby bindings aren't only for my project.
> > 
> > 
> > Thanks,
> > --
> > kou
> > 
> > ------------------------------------------------------
> > http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=994180
> >
> 
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=996360
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=996469