[PATCH v3] Tests for pthread_getcpuclockid method. Ticket:3891
Gedare Bloom
gedare at rtems.org
Fri Apr 17 18:08:33 UTC 2020
On Fri, Apr 17, 2020 at 11:48 AM Eshan Dhawan <eshandhawan51 at gmail.com> wrote:
>
>
>
> On Fri, Apr 17, 2020 at 9:49 PM Gedare Bloom <gedare at rtems.org> wrote:
>>
>> On Fri, Apr 17, 2020 at 8:31 AM Eshan dhawan <eshandhawan51 at gmail.com> wrote:
>> >
>> > ---
>> > testsuites/psxtests/Makefile.am | 10 ++
>> > testsuites/psxtests/configure.ac | 1 +
>> > testsuites/psxtests/psxgetcpuclockid01/init.c | 149 ++++++++++++++++++
>> > .../psxgetcpuclockid01/psxgetcpuclockid01.doc | 23 +++
>> > .../psxgetcpuclockid01/psxgetcpuclockid01.scn | 4 +
>> > 5 files changed, 187 insertions(+)
>> > create mode 100644 testsuites/psxtests/psxgetcpuclockid01/init.c
>> > create mode 100644 testsuites/psxtests/psxgetcpuclockid01/psxgetcpuclockid01.doc
>> > create mode 100644 testsuites/psxtests/psxgetcpuclockid01/psxgetcpuclockid01.scn
>> >
>> > diff --git a/testsuites/psxtests/Makefile.am b/testsuites/psxtests/Makefile.am
>> > index 1f9e4233ec..ffadff16e7 100755
>> > --- a/testsuites/psxtests/Makefile.am
>> > +++ b/testsuites/psxtests/Makefile.am
>> > @@ -462,6 +462,16 @@ psxgetattrnp01_CPPFLAGS = $(AM_CPPFLAGS) $(TEST_FLAGS_psxgetattrnp01) \
>> > $(support_includes) -I$(top_srcdir)/include
>> > endif
>> >
>> > +if TEST_psxgetcpuclockid01
>> > +psx_tests += psxgetcpuclockid01
>> > +psx_screens += psxgetcpuclockid01/psxgetcpuclockid01.scn
>> > +psx_docs += psxgetcpuclockid01/psxgetcpuclockid01.doc
>> > +psxgetcpuclockid01_SOURCES = psxgetcpuclockid01/init.c
>> > +psxgetcpuclockid01_CPPFLAGS = $(AM_CPPFLAGS) $(TEST_FLAGS_psxgetcpuclockid01) \
>> > + $(support_includes)
>> > +endif
>> > +
>> > +
>> extra blank
>>
>> > if TEST_psxgetrusage01
>> > psx_tests += psxgetrusage01
>> > psx_screens += psxgetrusage01/psxgetrusage01.scn
>> > diff --git a/testsuites/psxtests/configure.ac b/testsuites/psxtests/configure.ac
>> > index 139787cccb..c509086abc 100644
>> > --- a/testsuites/psxtests/configure.ac
>> > +++ b/testsuites/psxtests/configure.ac
>> > @@ -91,6 +91,7 @@ RTEMS_TEST_CHECK([psxfile01])
>> > RTEMS_TEST_CHECK([psxfile02])
>> > RTEMS_TEST_CHECK([psxfilelock01])
>> > RTEMS_TEST_CHECK([psxgetattrnp01])
>> > +RTEMS_TEST_CHECK([psxgetcpuclockid01])
>> > RTEMS_TEST_CHECK([psxgetrusage01])
>> > RTEMS_TEST_CHECK([psxglobalcon01])
>> > RTEMS_TEST_CHECK([psxglobalcon02])
>> > diff --git a/testsuites/psxtests/psxgetcpuclockid01/init.c b/testsuites/psxtests/psxgetcpuclockid01/init.c
>> > new file mode 100644
>> > index 0000000000..a2a6476d96
>> > --- /dev/null
>> > +++ b/testsuites/psxtests/psxgetcpuclockid01/init.c
>> > @@ -0,0 +1,149 @@
>> > +/*
>> > + * @file
>> > + * @brief Test suite for getcpuclockid methods
>> > + */
>> > +
>> > +/*
>> > + * SPDX-License-Identifier: BSD-2-Clause
>> > + *
>> > + * Copyright (C) 2020 Eshan Dhawan, Vaibhav Gupta
>> > + *
>> > + * Redistribution and use in source and binary forms, with or without
>> > + * modification, are permitted provided that the following conditions
>> > + * are met:
>> > + * 1. Redistributions of source code must retain the above copyright
>> > + * notice, this list of conditions and the following disclaimer.
>> > + * 2. Redistributions in binary form must reproduce the above copyright
>> > + * notice, this list of conditions and the following disclaimer in the
>> > + * documentation and/or other materials provided with the distribution.
>> > + *
>> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
>> > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
>> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
>> > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
>> > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
>> > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>> > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
>> > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
>> > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
>> > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
>> > + * POSSIBILITY OF SUCH DAMAGE.
>> > + */
>> > +
>> > +
>> extra blank
>> > +#ifdef HAVE_CONFIG_H
>> > +#include "config.h"
>> > +#endif
>> > +
>> > +#include <errno.h>
>> > +#include <pthread.h>
>> > +#include <stdint.h>
>> > +#include <stdio.h>
>> > +#include <rtems/test.h>
>> > +#include <time.h>
>> > +#include <unistd.h>
>> > +
>> > +#include "tmacros.h"
>> > +#include "test_support.h"
>> > +
>> > +const char rtems_test_name[] = "PSXGETCPUCLOCKID 1";
>> > +
>> > +/* Forward declaration to avoid warnings */
>> > +void *POSIX_Init (void * argument);
>> > +void *test_pthread(void *);
>> > +/*structure timespec */
>> > +
>> > +/* test_pthread function begins */
>> > +void *test_pthread(void *arg)
>> The fn name should reflect the job it does. This isn't testing pthread.
>> test_pthread_getcpuclockid()?
>>
>>
>> > +{
>> > + int r;
>> > + clockid_t cid;
>> > + struct timespec start_cpu, start_wall, finish;
>> > + long time_passed_wall, time_passed_cpu ;
>> > +
>> > + r = pthread_getcpuclockid( pthread_self(), &cid );
>> > + if ( r ) {
>> > + printf( "pthread_getcpuclockid : %d", r );
>> > + }
>> > + rtems_test_assert( r == 0 );
>> > +
>> > + r = clock_gettime( cid, &start_cpu );
>> > + if ( r ) {
>> > + printf( "clock_gettime : %d \n", r );
>> Maybe use %s and strerror(r)? There are other examples in the psxtests of this.
>>
>> > + if ( r == EINVAL || r == ESRCH ) {
>> > + printf( "the clockid returned is invalid" );
>> > + }
>> I don't think this is necessary especially if you print the strerror() version
>>
>> > + }
>> > + rtems_test_assert( r == 0 );
>> > +
>> > + r = clock_gettime( CLOCK_REALTIME, &start_wall );
>> > + if ( r ) {
>> > + printf( "clock_gettime : %d \n", r );
>> > + if ( r == EINVAL || r == ESRCH ) {
>> > + printf( "the clockid returned is invalid" );
>> > + }
>> ditto
>>
>> > + }
>> > + rtems_test_assert( r == 0 );
>> > + while (1) {
>> > + clock_gettime( CLOCK_REALTIME, &finish );
>> > + time_passed_wall = finish.tv_nsec - start_wall.tv_nsec ;
>> > + if(time_passed >= 500000000){
>> > + break;
>> Why break a while(1) loop on a condition?
>>
>> What is this testing?
>>
>> We would like tests to finish quickly whenever possible. How long
>> would this test expect to take, and is that necessary?
>>
> Here I tried to test the clock_id provided by the pthread_getcpuclockid() function
> this test was suggested by Dr Joel in the patch V1 so I just improved it in this version
> An I wasn't able to fully understand in the previous patch.
> you can read more details for the test in this mail
> https://lists.rtems.org/pipermail/devel/2020-April/059292.html
If you don't understand a request from someone, please ask them for
more details. It is a waste of everyone's time if you go write code
without understanding the requirements.
I also see that Joel complained that this test won't run. You need to
add such comments in your commit message or email introduction if a
patch is not expected to work yet, or if you haven't tested it
yourself. In general, sending us patches to review that you haven't
tested is also wasting our time. We don't like to waste our time.
>>
>> > + }
>> > + }
>> > + printf( "wall_time = %li\n", time_passed_wall );
>> > + clock_gettime( cid, &finish );
>> > + time_passed_cpu = finish.tv_nsec - start_cpu.tv_nsec ;
>> > + printf( "CPU_time = %li\n", time_passed );
>> > + pthread_exit(0);
>> > + return NULL;
>> dead code and not strictly correct.
>>
This "return NULL;" will never be executed. pthread_exit() does not return.
NULL has a type (void *). This function is declared void, it should
return nothing.
Thus, an explicit return statement should just be:
return;
while the fall-out of the end of the function can be simply the
closing french brace.
>> > +}
>> > +
>> > +/* init test function begins */
>> > +void *POSIX_Init (void * argument)
>> > +{
>> > + TEST_BEGIN();
>> > +
>> > + int r;
>> > + clockid_t cid;
>> > + pthread_t thread_id;
>> > +
>> > + r= pthread_create( &thread_id, NULL, test_pthread, NULL );
>> Why using another pthread?
>
> So that when I join that pthread and the thread_id becomes invalid and could be used for
> testing the ESRCH exception
I still don't understand why it is necessary to do this test in a
separate pthread.
>>
>> > + if( r )
>> > + {
Missed this before, put the { on same line as if ( r ) and check white
space after if.
>> > + printf( "pthread_create : %d", r );
>> > + }
>> > + rtems_test_assert( r == 0 );
>> > + /*
>> > + r = pthread_join( thread_id, NULL );
>> > + if( r )
>> > + {
>> > + printf( "pthread_join : %d", r );
>> > + }
>> > + rtems_test_assert( r == 0 );
>> > +
>> > + r = pthread_getcpuclockid( thread_id, &cid );
>> > + if( r != ESRCH )
>> > + {
>> > + printf( "pthread_getcpuclockid : %d", r );
>> > + }
>> > + rtems_test_assert( r == ESRCH );
>> > +*/
Missed this before also, do not comment-out code. Code is either
necessary or unnecessary.
>> > + TEST_END();
>> > + rtems_test_exit(0);
>> > +}
>> > +
>> > +#define CONFIGURE_APPLICATION_NEEDS_CLOCK_DRIVER
>> > +#define CONFIGURE_APPLICATION_NEEDS_SIMPLE_CONSOLE_DRIVER
>> > +
>> > +#define CONFIGURE_MAXIMUM_POSIX_THREADS 2
>> > +#define CONFIGURE_MAXIMUM_TASKS 1
>> > +
>> > +#define CONFIGURE_INITIAL_EXTENSIONS RTEMS_TEST_INITIAL_EXTENSION
>> > +
>> > +#define CONFIGURE_POSIX_INIT_THREAD_TABLE
>> > +
>> > +#define CONFIGURE_INIT
>> > +
>> > +#include <rtems/confdefs.h>
>> > +
>> > diff --git a/testsuites/psxtests/psxgetcpuclockid01/psxgetcpuclockid01.doc b/testsuites/psxtests/psxgetcpuclockid01/psxgetcpuclockid01.doc
>> > new file mode 100644
>> > index 0000000000..ef148182d5
>> > --- /dev/null
>> > +++ b/testsuites/psxtests/psxgetcpuclockid01/psxgetcpuclockid01.doc
>> > @@ -0,0 +1,23 @@
>> > +# COPYRIGHT (c) 2020
>> > +# On-Line Applications Research Corporation (OAR).
>> > +#
>> > +# SPDX-License-Identifier: BSD-2-Clause
>> > +#
>> > +
>> > +This file describes the directives and concepts tested by this test set.
>> > +
>> > +test set name: psxgetcpuclockid 1
>> > +
>> > +Directives:
>> > + Pthread_getcpuclockid()
>> > +
>> > +Concepts:
>> > +
>> > ++ This test exercises the getcpuclockid methods.
>> > +
>> > ++ For pthread_getcpuclockid it created a pthread using pthread_create and
>> > + then finds the clockid_t of that thread then used gettime() add a pause of .5 seconds
>> > + and then ends the thread.
>> > + After the thread is ended it again tried to find the clockid_t of the thread to
>> > + check for ESRCH exception.
>> > +
>> > diff --git a/testsuites/psxtests/psxgetcpuclockid01/psxgetcpuclockid01.scn b/testsuites/psxtests/psxgetcpuclockid01/psxgetcpuclockid01.scn
>> > new file mode 100644
>> > index 0000000000..937a908fd5
>> > --- /dev/null
>> > +++ b/testsuites/psxtests/psxgetcpuclockid01/psxgetcpuclockid01.scn
>> > @@ -0,0 +1,4 @@
>> > +*** BEGIN OF TEST PSXGETCLOCKID 1 ***
>> > +
>> > +*** END OF TEST PSXGETCPUCLOCKID 1 ***
>> > +
>> > --
>> > 2.17.1
>> >
More information about the devel
mailing list