You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2019/10/17 21:48:28 UTC

Re: svn commit: r1868561 - /subversion/trunk/subversion/tests/cmdline/diff_tests.py

Good morning Nathan,

hartmannathan@apache.org wrote on Thu, Oct 17, 2019 at 16:23:21 -0000:
> +++ subversion/trunk/subversion/tests/cmdline/diff_tests.py Thu Oct 17 16:23:21 2019
> @@ -5253,6 +5253,83 @@ def diff_git_format_copy(sbox):
> +def diff_nonexistent_in_wc(sbox):
> +  "nonexistent in working copy"
> +
> +  sbox.build(empty=True)
> +  wc_dir = sbox.wc_dir
> +
> +  # We mirror the actions of the reproduction script (with one exception:
> +  # we 'svn up -r 0' instead of checking out a second working copy)
> +
> +  sbox.simple_add_text('test\n', 'file')
> +  sbox.simple_commit()

If instead of using «echo test > file» you would use the existing file 'iota'
(see subversion/tests/README), you'd be able to pass read_only=True to
sbox.build(), which avoids some test suite overhead in setting a dedicated
repository for each test function — i.e., makes tests run half a millisecond faster. ☺

Cheers,

Daniel

Re: svn commit: r1868561 - /subversion/trunk/subversion/tests/cmdline/diff_tests.py

Posted by Nathan Hartman <ha...@gmail.com>.
On Fri, Oct 18, 2019 at 12:37 AM Daniel Shahaf <d....@daniel.shahaf.name>
wrote:

> Nathan Hartman wrote on Fri, Oct 18, 2019 at 00:08:48 -0400:
> > On Thu, Oct 17, 2019 at 5:48 PM Daniel Shahaf <d....@daniel.shahaf.name>
> > wrote:
> >
> > > Good morning Nathan,
> > >
> >
> > Good morning! Or whatever time it happens to be in your part of the world
> > :-)
>
> <https://en.wikipedia.org/wiki/UGT>, fifth sense.


That makes sense.

In fact, I wonder what would happen if the test were done in A/B/E. The
> difference is that the working copy root does exist at r0, but A/B/E
> doesn't.


That should be an interesting test case. I'll try a few diffs with A/B/E/
manually with the cmdline client just to see what happens.

But as Julian said, we're entering the territory of diminishing returns, so
> it's up to you whether to continue ironing this one out or move on to other
> issues.


I agree. We're beating a dead horse here.

I'll proceed to locate the next old issue that should be looked at...

Re: svn commit: r1868561 - /subversion/trunk/subversion/tests/cmdline/diff_tests.py

Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Nathan Hartman wrote on Fri, Oct 18, 2019 at 00:08:48 -0400:
> On Thu, Oct 17, 2019 at 5:48 PM Daniel Shahaf <d....@daniel.shahaf.name>
> wrote:
> 
> > Good morning Nathan,
> >
> 
> Good morning! Or whatever time it happens to be in your part of the world
> :-)

<https://en.wikipedia.org/wiki/UGT>, fifth sense.

> Unfortunately, instead of reproducing the output shown in SVN-1722,
> it now produces a much longer diff that includes all the files, some
> 60+ lines, and consequently fails the test, but that could be fixed,
> but...
> 
> While I could run 'svn diff -r BASE:HEAD iota' etc., I think that
> would be the wrong thing to do because SVN-1722 is so old that we
> don't know exactly what circumstances used to trigger it. What if
> we change the test to save some milliseconds and end up defeating
> the purpose? I don't know. Do you think it's worth it?

Yes, good point: adding 'iota' there could invalidate the test.  The
bug may well have something to do with how the positional arguments,
peg revision, and operative revisions are resolved to an anchor/target
pair for the edit operation [in the sense of an svn_delta_editor_t drive]
in edge cases, such as when the target doesn't exist in one operative revision.

In fact, I wonder what would happen if the test were done in A/B/E. The
difference is that the working copy root does exist at r0, but A/B/E doesn't.

But as Julian said, we're entering the territory of diminishing returns, so
it's up to you whether to continue ironing this one out or move on to other
issues.

Cheers,

Daniel

Re: svn commit: r1868561 - /subversion/trunk/subversion/tests/cmdline/diff_tests.py

Posted by Nathan Hartman <ha...@gmail.com>.
On Thu, Oct 17, 2019 at 5:48 PM Daniel Shahaf <d....@daniel.shahaf.name>
wrote:

> Good morning Nathan,
>

Good morning! Or whatever time it happens to be in your part of the world
:-)

If instead of using «echo test > file» you would use the existing file
> 'iota'
> (see subversion/tests/README), you'd be able to pass read_only=True to
> sbox.build(), which avoids some test suite overhead in setting a dedicated
> repository for each test function — i.e., makes tests run half a
> millisecond faster. ☺


Thank you for this suggestion!

So I tried this:

[[[

#----------------------------------------------------------------------
# Regression test for issue #1722: 'svn diff' produced a wrong header,
# indicating one revision as being in the working copy when it should
# be 'nonexistent'
@Issue(1722)
def diff_nonexistent_in_wc(sbox):
  "nonexistent in working copy"

  sbox.build(read_only=True)
  wc_dir = sbox.wc_dir

  sbox.simple_update(revision=0)

  # Expected output is empty for these cases:
  # svn diff -r BASE
  # svn diff -r 0:BASE
  # svn diff -r 0

  # Expected output for:
  # svn diff -r BASE:HEAD
  # svn diff -r 0:HEAD
  # svn diff -r 0:1
  expected_output_base_head = make_diff_header("iota", "nonexistent",
                                               "revision 1") + [
  "@@ -0,0 +1 @@\n",
  "+This is the file 'iota'.\n",
  ]

  # Expected output for:
  # svn diff -r HEAD:BASE
  # svn diff -r HEAD
  # svn diff -r 1:0
  # svn diff -r 1
  expected_output_head_base = make_diff_header("iota", "revision 1",
                                               "nonexistent") + [
  "@@ -1 +0,0 @@\n",
  "-This is the file 'iota'.\n"
  ]

  os.chdir(wc_dir)

  svntest.actions.run_and_verify_svn(expected_output_base_head, [],
                                     'diff', '-r', 'BASE:HEAD')

  svntest.actions.run_and_verify_svn(expected_output_head_base, [],
                                     'diff', '-r', 'HEAD:BASE')

  svntest.actions.run_and_verify_svn([], [],
                                     'diff', '-r', 'BASE')

  svntest.actions.run_and_verify_svn(expected_output_head_base, [],
                                     'diff', '-r', 'HEAD')

  svntest.actions.run_and_verify_svn([], [],
                                     'diff', '-r', '0:BASE')

  svntest.actions.run_and_verify_svn(expected_output_base_head, [],
                                     'diff', '-r', '0:HEAD')

  svntest.actions.run_and_verify_svn(expected_output_base_head, [],
                                     'diff', '-r', '0:1')

  svntest.actions.run_and_verify_svn(expected_output_head_base, [],
                                     'diff', '-r', '1:0')

  svntest.actions.run_and_verify_svn([], [],
                                     'diff', '-r', '0')

  svntest.actions.run_and_verify_svn(expected_output_head_base, [],
                                     'diff', '-r', '1')

]]]

I like shorter code!

Unfortunately, instead of reproducing the output shown in SVN-1722,
it now produces a much longer diff that includes all the files, some
60+ lines, and consequently fails the test, but that could be fixed,
but...

While I could run 'svn diff -r BASE:HEAD iota' etc., I think that
would be the wrong thing to do because SVN-1722 is so old that we
don't know exactly what circumstances used to trigger it. What if
we change the test to save some milliseconds and end up defeating
the purpose? I don't know. Do you think it's worth it?

Nathan