Re: svn commit: r14898 - in trunk/subversion: tests/clients/cmdline

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r14898 - in trunk/subversion: tests/clients/cmdline

Philip Martin
[hidden email] writes:

> Author: lundblad
> Date: Tue May 31 16:40:03 2005
> New Revision: 14898

I'm in favour of regression tests, but this one could be better.

> +def lock_uri_encoded(sbox):
> +  "lock and unlock a file with an URI-unsafe name"
> +
> +  sbox.build()
> +  wc_dir = sbox.wc_dir
> +
> +  # lock a file as wc_author
> +  fname = 'amazing space'
> +  file_path = os.path.join(sbox.wc_dir, fname)
> +
> +  svntest.main.file_append(file_path, "This represents a binary file\n")
> +  svntest.main.run_svn(None, "add", file_path)
> +  svntest.main.run_svn(None, 'commit',
> +                       '--username', svntest.main.wc_author,
> +                       '--password', svntest.main.wc_passwd,
> +                       '-m', '', file_path)

run_svn does almost no error checking, run_and_verify_commit would be
much better.  I suppose you could claim that the subsequent lock will
ensure that the commit worked, except...

> +  svntest.actions.run_and_verify_svn(None, None, None, 'lock',
> +                                     '--username', svntest.main.wc_author,
> +                                     '--password', svntest.main.wc_passwd,
> +                                     '-m', '', file_path)

...there is almost no error checking here either.  run_and_verify_svn
is a bit better than run_svn but it still doesn't really verify that a
lock got created.  Using 'svn info' or run_and_verify_status would be
an improvement.

> +
> +
> +  svntest.actions.run_and_verify_svn(None, None, None, 'unlock',
> +                                     '--username', svntest.main.wc_author,
> +                                     '--password', svntest.main.wc_passwd,
> +                                     file_path)

This doesn't verify that the lock got released.

--
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r14898 - in trunk/subversion: tests/clients/cmdline

Peter N. Lundblad
On Wed, 1 Jun 2005, Philip Martin wrote:

> [hidden email] writes:
>
> > Author: lundblad
> > Date: Tue May 31 16:40:03 2005
> > New Revision: 14898
>
> > +def lock_uri_encoded(sbox):
> > +  "lock and unlock a file with an URI-unsafe name"
> > +
> > +  sbox.build()
> > +  wc_dir = sbox.wc_dir
> > +
> > +  # lock a file as wc_author
> > +  fname = 'amazing space'
> > +  file_path = os.path.join(sbox.wc_dir, fname)
> > +
> > +  svntest.main.file_append(file_path, "This represents a binary file\n")
> > +  svntest.main.run_svn(None, "add", file_path)
> > +  svntest.main.run_svn(None, 'commit',
> > +                       '--username', svntest.main.wc_author,
> > +                       '--password', svntest.main.wc_passwd,
> > +                       '-m', '', file_path)
>
> run_svn does almost no error checking, run_and_verify_commit would be
> much better.  I suppose you could claim that the subsequent lock will
> ensure that the commit worked, except...
>
> > +  svntest.actions.run_and_verify_svn(None, None, None, 'lock',
> > +                                     '--username', svntest.main.wc_author,
> > +                                     '--password', svntest.main.wc_passwd,
> > +                                     '-m', '', file_path)
>
> ...there is almost no error checking here either.  run_and_verify_svn
> is a bit better than run_svn but it still doesn't really verify that a
> lock got created.  Using 'svn info' or run_and_verify_status would be
> an improvement.
>
I'll take a look.

> > +
> > +
> > +  svntest.actions.run_and_verify_svn(None, None, None, 'unlock',
> > +                                     '--username', svntest.main.wc_author,
> > +                                     '--password', svntest.main.wc_passwd,
> > +                                     file_path)
>
> This doesn't verify that the lock got released.
>
>
But it verifies that there was no error output.  Ain't that enough?  (BTW,
looking at the break_lock test, that might have the same problem)?

Thanks,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r14898 - in trunk/subversion: tests/clients/cmdline

Brian W. Fitzpatrick-2

On Jun 2, 2005, at 2:47 AM, Peter N. Lundblad wrote:

> On Wed, 1 Jun 2005, Philip Martin wrote:
>
>
>>> +
>>> +
>>> +  svntest.actions.run_and_verify_svn(None, None, None, 'unlock',
>>> +                                     '--username',  
>>> svntest.main.wc_author,
>>> +                                     '--password',  
>>> svntest.main.wc_passwd,
>>> +                                     file_path)
>>>
>>
>> This doesn't verify that the lock got released.
>>
>>
>>
> But it verifies that there was no error output.  Ain't that  
> enough?  (BTW,
> looking at the break_lock test, that might have the same problem)?

I think that we need to rejigger all the locking tests to  
run_and_verify status after locking/unlocking.  We've already seen  
several situations where svn segfaults and, since nothing showed up  
on STDERR, the test succeeded.

-Fitz

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r14898 - in trunk/subversion: tests/clients/cmdline

Philip Martin
In reply to this post by Peter N. Lundblad
"Peter N. Lundblad" <[hidden email]> writes:

> On Wed, 1 Jun 2005, Philip Martin wrote:
>
>> [hidden email] writes:
>
>> > +
>> > +
>> > +  svntest.actions.run_and_verify_svn(None, None, None, 'unlock',
>> > +                                     '--username', svntest.main.wc_author,
>> > +                                     '--password', svntest.main.wc_passwd,
>> > +                                     file_path)
>>
>> This doesn't verify that the lock got released.
>>
>>
> But it verifies that there was no error output.  Ain't that enough?

Not really.  A recent change caused 'svn unlock URL' to SEGV and the
the test using it still passed.  As it happens the SEGV occurred after
the lock was removed, but if it had happened before the lock was
removed the test would still have passed.

> (BTW, looking at the break_lock test, that might have the same
> problem)?

Very likely, lots of the locking tests have similar problems.

--
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r14898 - in trunk/subversion: tests/clients/cmdline

Jani Averbach
On 2005-06-02 17:20+0100, Philip Martin wrote:

> "Peter N. Lundblad" <[hidden email]> writes:
>
> > On Wed, 1 Jun 2005, Philip Martin wrote:
> >
> >> [hidden email] writes:
> >
> >> > +
> >> > +
> >> > +  svntest.actions.run_and_verify_svn(None, None, None, 'unlock',
> >> > +                                     '--username', svntest.main.wc_author,
> >> > +                                     '--password', svntest.main.wc_passwd,
> >> > +                                     file_path)
> >>
> >> This doesn't verify that the lock got released.
> >>
> >>
> > But it verifies that there was no error output.  Ain't that enough?
>
> Not really.  A recent change caused 'svn unlock URL' to SEGV and the
> the test using it still passed.  As it happens the SEGV occurred after
> the lock was removed, but if it had happened before the lock was
> removed the test would still have passed.

I will try to include and use subprocess[1] with our test framework
and see if it makes any difference with 'returncode'. This is new in
Python 2.4 and it won't probably help with win32, so coverage won't be
100%, but at least it might help a little.
 
BR, Jani

1) http://docs.python.org/lib/module-subprocess.html

--
Jani Averbach

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r14898 - in trunk/subversion: tests/clients/cmdline

Philip Martin
Jani Averbach <[hidden email]> writes:

> On 2005-06-02 17:20+0100, Philip Martin wrote:
>>
>> Not really.  A recent change caused 'svn unlock URL' to SEGV and the
>> the test using it still passed.  As it happens the SEGV occurred after
>> the lock was removed, but if it had happened before the lock was
>> removed the test would still have passed.
>
> I will try to include and use subprocess[1] with our test framework
> and see if it makes any difference with 'returncode'. This is new in
> Python 2.4 and it won't probably help with win32, so coverage won't be
> 100%, but at least it might help a little.

Are you proposing to detect SEGVs?  This patch

http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=34876

was not applied because it didn't support win32.

--
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Detecting SIGSEGVs with the test framework

Jani Averbach
On 2005-06-02 18:28+0100, Philip Martin wrote:

> Jani Averbach <[hidden email]> writes:
>
> > On 2005-06-02 17:20+0100, Philip Martin wrote:
> >>
> >> Not really.  A recent change caused 'svn unlock URL' to SEGV and the
> >> the test using it still passed.  As it happens the SEGV occurred after
> >> the lock was removed, but if it had happened before the lock was
> >> removed the test would still have passed.
> >
> > I will try to include and use subprocess[1] with our test framework
> > and see if it makes any difference with 'returncode'. This is new in
> > Python 2.4 and it won't probably help with win32, so coverage won't be
> > 100%, but at least it might help a little.
>
> Are you proposing to detect SEGVs?  This patch
>
> http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=34876

The whole thread is here:
http://svn.haxx.se/dev/archive-2003-04/0391.shtml

> was not applied because it didn't support win32.

Please propose it again, and here is my rationale why:

We have had several SIGSEV cases during last six months and almost all
the time they have been lurking a while in the code base until they
have dragged to sunlight.

If there is some ?ber cool way to detect errors on win32, let's use
it! If there is some clever way to check correctness on OS X, same
here, use it.  And if there is some way to detect SIGSEVs on UNIX,
please, use it.  We have a portable code base, it is a strength, and
use that richness to comb out automatically by all possible means as
many bugs and defects as possible.

We can enforce our log format without any pre-commit-hook, and I am
pretty sure that we could enforce the old way to write non-sloppy test
cases. Every time when this SIGSEV detector found a case which could have
been detected by ordinary test setup, but which isn't, then
responsible party or parties could be flogged or dragged under keel.

BR, Jani

--
Jani Averbach

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r14898 - in trunk/subversion: tests/clients/cmdline

Peter N. Lundblad
In reply to this post by Philip Martin
On Thu, 2 Jun 2005, Philip Martin wrote:

> "Peter N. Lundblad" <[hidden email]> writes:
>
> > On Wed, 1 Jun 2005, Philip Martin wrote:
> >
> >> [hidden email] writes:
> >
> > But it verifies that there was no error output.  Ain't that enough?
>
> Not really.  A recent change caused 'svn unlock URL' to SEGV and the
> the test using it still passed.  As it happens the SEGV occurred after
> the lock was removed, but if it had happened before the lock was
> removed the test would still have passed.
>
Yeah, fitz also pointed this out, and it makes sense.  It's a pity that
the test harnest doesn't bail on abnormal exits, but anyway...

Thanks for clarifying,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]