Re: [ros-dev] [ros-diffs] [greatlrd] 33807: add Test for EngDeleteSemaphore, it only test if it been create or not

[Alex Ionescu]

I'm getting *quite* annoyed with seeing these kinds of tests:

ret = SomeFunc();
ASSERT(ret == success);

How exactly is this a test anyway? It proves in NO way that the
function *worked*, only that it returned success. Furthermore, there
are *plenty* of cases where a function could legitimately fail -- it
is simply stupid to assume that a failure means a regression. What if
the system is out of memory?

On Sun, Jun 1, 2008 at 10:38 PM,  <greatlrd(a)svn.reactos.org> wrote:
>   Author: greatlrd
>  Date: Sun Jun  1 09:38:02 2008
>  New Revision: 33807
>
>  URL: http://svn.reactos.org/svn/reactos?rev=33807&view=rev
>  Log:
>  add Test for EngDeleteSemaphore, it only test if it been create or not
>
>  Added:
>     trunk/rostests/apitests/gdi32api/tests/EngCreateSemaphore.c   (with props)
>  Modified:
>     trunk/rostests/apitests/gdi32api/testlist.c
>
>  Modified: trunk/rostests/apitests/gdi32api/testlist.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/rostests/apitests/gdi32api/testlis…
>  ==============================================================================
>  --- trunk/rostests/apitests/gdi32api/testlist.c [iso-8859-1] (original)
>  +++ trunk/rostests/apitests/gdi32api/testlist.c [iso-8859-1] Sun Jun  1 09:38:02 2008
>  @@ -11,6 +11,7 @@
>   #include "tests/CreateFont.c"
>   #include "tests/CreatePen.c"
>   #include "tests/CreateRectRgn.c"
>  +#include "tests/EngCreateSemaphore.c"
>   #include "tests/ExtCreatePen.c"
>   #include "tests/GdiConvertBitmap.c"
>   #include "tests/GdiConvertBrush.c"
>  @@ -51,6 +52,7 @@
>         { L"CreateCompatibleDC", Test_CreateCompatibleDC },
>         { L"CreateFont", Test_CreateFont },
>         { L"CreatePen", Test_CreatePen },
>  +       { L"EngCreateSemaphore", Test_EngCreateSemaphore },
>         { L"CreateRectRgn", Test_CreateRectRgn },
>         { L"ExtCreatePen", Test_ExtCreatePen },
>         { L"GdiConvertBitmap", Test_GdiConvertBitmap },
>
>  Added: trunk/rostests/apitests/gdi32api/tests/EngCreateSemaphore.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/rostests/apitests/gdi32api/tests/E…
>  ==============================================================================
>  --- trunk/rostests/apitests/gdi32api/tests/EngCreateSemaphore.c (added)
>  +++ trunk/rostests/apitests/gdi32api/tests/EngCreateSemaphore.c [iso-8859-1] Sun Jun  1
> 09:38:02 2008
>  @@ -1,0 +1,15 @@
>  +/* Simple test of EngAcquireSemaphore only check if we got a lock or not */
>  +INT
>  +Test_EngCreateSemaphore(PTESTINFO pti)
>  +{
>  +
>  +    HSEMAPHORE hsem;
>  +    hsem = EngCreateSemaphore();
>  +
>  +    RTEST ( hsem != NULL );
>  +
>  +    EngDeleteSemaphore(hsem);
>  +
>  +    return APISTATUS_NORMAL;
>  +}
>  +
>
>  Propchange: trunk/rostests/apitests/gdi32api/tests/EngCreateSemaphore.c
>  ------------------------------------------------------------------------------
>     svn:eol-style = native
>
>  


-- 
Best regards,
Alex Ionescu

[Timo Kreuzer]

And I'm getting *fairly* annoyed of smart alecks.

I'll explain it to you.
First, whenever you see an ASSERT(), it's not a test.
It just doesn't make sense to continue testing when the ASSERT fails, so 
the test is quitted.

Then there are cases like
ret = SomeFunc(Param1, Param2);
TEST(ret == success);
or
TEST(ret != success);

It doesn't test that the function is implemented propertly, it just 
tests the handling of valid/invalid parameters.

The case from below, is clearly not a good example for this, as it 
doesn't have any parameters. But have you ever thought that this might 
be work in progress and more things are to come? The test ist just a 
simple way to see whether "it worked at all". If the test fails, you 
know the function completely failed, when it - normally - shouldn't.

And please stop that stupid "the function may fail, when there's no more 
system memory" argument. That's silly. We cannot handle these cases, and 
it doesn't make sense. The tests are designated to be executed on a 
"clean" system they are not thought as system stress tests.

And, no, the tests *proof* nothing, they are damn tests to help us find 
bugs. If you want proofs, write 10 pages of source code documentation 
that proof the correctness...

Alex Ionescu wrote:
>   I'm getting *quite* annoyed with seeing these
> kinds of tests:
>
>  ret = SomeFunc();
>  ASSERT(ret == success);
>
>  How exactly is this a test anyway? It proves in NO way that the
>  function *worked*, only that it returned success. Furthermore, there
>  are *plenty* of cases where a function could legitimately fail -- it
>  is simply stupid to assume that a failure means a regression. What if
>  the system is out of memory?
>
>  On Sun, Jun 1, 2008 at 10:38 PM,  <greatlrd(a)svn.reactos.org> wrote:
>    
> >   Author: greatlrd
> >  Date: Sun Jun  1 09:38:02 2008
> >  New Revision: 33807
> >
> >  URL: http://svn.reactos.org/svn/reactos?rev=33807&view=rev
> >  Log:
> >  add Test for EngDeleteSemaphore, it only test if it been create or not
> >
> >  Added:
> >     trunk/rostests/apitests/gdi32api/tests/EngCreateSemaphore.c   (with props)
> >  Modified:
> >     trunk/rostests/apitests/gdi32api/testlist.c
> >
> >  Modified: trunk/rostests/apitests/gdi32api/testlist.c
> >  URL:
> > http://svn.reactos.org/svn/reactos/trunk/rostests/apitests/gdi32api/testlis…
> >  ==============================================================================
> >  --- trunk/rostests/apitests/gdi32api/testlist.c [iso-8859-1] (original)
> >  +++ trunk/rostests/apitests/gdi32api/testlist.c [iso-8859-1] Sun Jun  1 09:38:02 2008
> >  @@ -11,6 +11,7 @@
> >   #include "tests/CreateFont.c"
> >   #include "tests/CreatePen.c"
> >   #include "tests/CreateRectRgn.c"
> >  +#include "tests/EngCreateSemaphore.c"
> >   #include "tests/ExtCreatePen.c"
> >   #include "tests/GdiConvertBitmap.c"
> >   #include "tests/GdiConvertBrush.c"
> >  @@ -51,6 +52,7 @@
> >         { L"CreateCompatibleDC", Test_CreateCompatibleDC },
> >         { L"CreateFont", Test_CreateFont },
> >         { L"CreatePen", Test_CreatePen },
> >  +       { L"EngCreateSemaphore", Test_EngCreateSemaphore },
> >         { L"CreateRectRgn", Test_CreateRectRgn },
> >         { L"ExtCreatePen", Test_ExtCreatePen },
> >         { L"GdiConvertBitmap", Test_GdiConvertBitmap },
> >
> >  Added: trunk/rostests/apitests/gdi32api/tests/EngCreateSemaphore.c
> >  URL:
> > http://svn.reactos.org/svn/reactos/trunk/rostests/apitests/gdi32api/tests/E…
> >  ==============================================================================
> >  --- trunk/rostests/apitests/gdi32api/tests/EngCreateSemaphore.c (added)
> >  +++ trunk/rostests/apitests/gdi32api/tests/EngCreateSemaphore.c [iso-8859-1] Sun Jun  1
> > 09:38:02 2008
> >  @@ -1,0 +1,15 @@
> >  +/* Simple test of EngAcquireSemaphore only check if we got a lock or not */
> >  +INT
> >  +Test_EngCreateSemaphore(PTESTINFO pti)
> >  +{
> >  +
> >  +    HSEMAPHORE hsem;
> >  +    hsem = EngCreateSemaphore();
> >  +
> >  +    RTEST ( hsem != NULL );
> >  +
> >  +    EngDeleteSemaphore(hsem);
> >  +
> >  +    return APISTATUS_NORMAL;
> >  +}
> >  +
> >
> >  Propchange: trunk/rostests/apitests/gdi32api/tests/EngCreateSemaphore.c
> >  ------------------------------------------------------------------------------
> >     svn:eol-style = native
> >
> >
> >       
>
>
>