Please use knownfail rather than skip

[Andrew Bartlett]

On Thu, 2013-01-31 at 19:40 +0100, Andreas Schneider wrote:
> The branch, master has been updated
>        via  cf27c2f selftest: skip smb2.ioctl tests on ntvfs
>       from  dc929ca tevent: Fix a comment typo
> 
> http://gitweb.samba.org/?p=samba.git;a=shortlog;h=master
> 
> 
> - Log -----------------------------------------------------------------
> commit cf27c2fbb6e7422cb962f4c63a53515321c65a70
> Author: David Disseldorp <ddiss at samba.org>
> Date:   Thu Jan 31 17:48:25 2013 +0100
> 
>     selftest: skip smb2.ioctl tests on ntvfs
>     
>     Rather than filtering via knownfail.
>     
>     Reviewed-by: Andreas Schneider <asn at samba.org>
>     
>     Autobuild-User(master): Andreas Schneider <asn at cryptomilk.org>
>     Autobuild-Date(master): Thu Jan 31 19:39:25 CET 2013 on sn-devel-104
> 
> -----------------------------------------------------------------------
> 
> Summary of changes:
>  selftest/knownfail |    2 --
>  selftest/skip      |    1 +
>  2 files changed, 1 insertions(+), 2 deletions(-)
> 
> 
> Changeset truncated at 500 lines:
> 
> diff --git a/selftest/knownfail b/selftest/knownfail
> index 39485af..dcd94ec 100644
> --- a/selftest/knownfail
> +++ b/selftest/knownfail
> @@ -172,8 +172,6 @@
>  ^samba3.smb2.durable-v2-open.open-lease
>  ^samba3.smb2.durable-v2-open.persistent-open-lease
>  ^samba3.smb2.durable-v2-open.app-instance
> -^samba4.smb2.ioctl.req_resume_key\(dc\) # not supported by s4 ntvfs server
> -^samba4.smb2.ioctl.copy_chunk_\w*\(dc\)	# not supported by s4 ntvfs server
>  ^samba3.smb2.dir.one
>  ^samba3.smb2.dir.modify
>  ^samba3.smb2.lease.request
> diff --git a/selftest/skip b/selftest/skip
> index 5c49306..d54a5b0 100644
> --- a/selftest/skip
> +++ b/selftest/skip
> @@ -61,6 +61,7 @@
>  ^samba4.smb2.dir
>  ^samba4.smb2.session
>  ^samba4.smb2.compound
> +^samba4.smb2.ioctl	# not supported by ntvfs
>  ^samba4.ntvfs.cifs.*.base.charset
>  ^samba4.ntvfs.cifs.*.base.iometer
>  ^samba4.ntvfs.cifs.*.base.casetable

G'day,

There may be more behind this than in obvious by this patch, but I need
to call this out as being a bad pattern to start. 

Unless there is some entirely undesirable side-effect, this patch throws
away valuable testing, because it removes the test of the testsuite for
failure (ie, does the test work!), and it removes the documentation of
the difference between the servers (if someone did add some this to the
ntvfs server, it would never be tested). 

It also removes a test that if this unimplemented functionality is
called, that we don't crash and die.

There are sometimes good reasons to do things like this - test time for
example - but just because something isn't supported right now shouldn't
be enough to skip a test that we can otherwise cheaply run.

Perhaps I missed the tread discussing the background, but I can't see
it.

Can you shed some light on the background here, or can we otherwise go
back to testing this?

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org