On Wed, Sep 16, 2015 at 05:03:38PM +0200, Christophe Milard wrote:
> On 2015-09-16 10:42, Stuart Haslam wrote:
> > Add an odp_cunit_update() function to allow some attributes of previously
> > registered tests to be modified, such as whether it's active or not.
> >
> > Previously, registering and running tests was done in a single step;
> >
> > odp_cunit_run(testsuites);
> >
> > But this is now split;
> >
> > odp_cunit_register(testsuites);
> > odp_cunit_update(testsuites_updates);
> > odp_cunit_run();
> >
> > The odp_cunit_update() is optional and none of the currently defined
> > tests use it, so there's no functional change in this patch.
> >
> > Signed-off-by: Stuart Haslam <[email protected]>
> > ---
> > test/validation/buffer/buffer.c | 7 +-
> > test/validation/classification/classification.c | 7 +-
> > test/validation/common/odp_cunit_common.c | 107
> > ++++++++++++++++++++++--
> > test/validation/common/odp_cunit_common.h | 11 ++-
> > test/validation/cpumask/cpumask.c | 7 +-
> > test/validation/crypto/crypto.c | 10 ++-
> > test/validation/errno/errno.c | 7 +-
> > test/validation/init/init.c | 29 ++++++-
> > test/validation/packet/packet.c | 7 +-
> > test/validation/pktio/pktio.c | 7 +-
> > test/validation/pool/pool.c | 7 +-
> > test/validation/queue/queue.c | 7 +-
> > test/validation/random/random.c | 7 +-
> > test/validation/scheduler/scheduler.c | 7 +-
> > test/validation/shmem/shmem.c | 7 +-
> > test/validation/synchronizers/synchronizers.c | 10 ++-
> > test/validation/system/system.c | 7 +-
> > test/validation/thread/thread.c | 7 +-
> > test/validation/time/time.c | 7 +-
> > test/validation/timer/timer.c | 7 +-
> > 20 files changed, 245 insertions(+), 27 deletions(-)
> >
> > diff --git a/test/validation/buffer/buffer.c
> > b/test/validation/buffer/buffer.c
> > index 4600e59..257e95c 100644
> > --- a/test/validation/buffer/buffer.c
> > +++ b/test/validation/buffer/buffer.c
> > @@ -153,5 +153,10 @@ odp_suiteinfo_t buffer_suites[] = {
> >
> > int buffer_main(void)
> > {
> > - return odp_cunit_run(buffer_suites);
> > + int ret = odp_cunit_register(buffer_suites);
> > +
> > + if (ret == 0)
> > + odp_cunit_run();
> > +
> > + return ret;
> > }
> > diff --git a/test/validation/classification/classification.c
> > b/test/validation/classification/classification.c
> > index b868c61..fe9a827 100644
> > --- a/test/validation/classification/classification.c
> > +++ b/test/validation/classification/classification.c
> > @@ -23,5 +23,10 @@ odp_suiteinfo_t classification_suites[] = {
> >
> > int classification_main(void)
> > {
> > - return odp_cunit_run(classification_suites);
> > + int ret = odp_cunit_register(classification_suites);
> > +
> > + if (ret == 0)
> > + ret = odp_cunit_run();
> > +
> > + return ret;
> > }
> > diff --git a/test/validation/common/odp_cunit_common.c
> > b/test/validation/common/odp_cunit_common.c
> > index d63e87e..8b62370 100644
> > --- a/test/validation/common/odp_cunit_common.c
> > +++ b/test/validation/common/odp_cunit_common.c
> > @@ -164,16 +164,99 @@ static int cunit_register_suites(odp_suiteinfo_t
> > testsuites[])
> > return 0;
> > }
> >
> > +static int cunit_update_test(odp_suiteinfo_t *sinfo,
> > + odp_testinfo_t *updated_tinfo)
> > +{
> > + odp_testinfo_t *tinfo;
> > +
> > + for (tinfo = sinfo->pTests; tinfo->testinfo.pName; tinfo++)
> > + if (strcmp(tinfo->testinfo.pName,
> > + updated_tinfo->testinfo.pName) == 0)
> > + break;
> > +
> > + if (!tinfo || !tinfo->testinfo.pName)
> > + return -1;
> > +
> > + tinfo->check_active = updated_tinfo->check_active;
> > +
> > + return 0;
> > +}
>
> Is the above function meant to update the tests or the test activity?
> It looks like it only update the test activity...
>
This is just updating the local properties of the test (i.e. stuff
that's in odp_testinfo_t but not in CU_TestInfo). The test function
itself is modified by the call to CU_set_test_func() before this
function is called. I'll rework it a little to make that clearer
though.
> > +
> > +static int cunit_update_suite(odp_suiteinfo_t *updated_sinfo)
> > +{
> > + CU_pSuite suite;
> > + CU_pTest test;
> > + odp_suiteinfo_t *sinfo;
> > + odp_testinfo_t *tinfo;
> > +
> > + /* find previously registered suite with matching name */
> > + for (sinfo = global_testsuites; sinfo->pName; sinfo++)
> > + if (strcmp(sinfo->pName, updated_sinfo->pName) == 0)
> > + break;
> > +
> > + if (!sinfo || !sinfo->pName) {
> > + fprintf(stderr, "%s: unable to add new suite: %s\n",
>
> shouldn't it say "unable to find existing suite with matching name"?
>
OK. I was supposing that this would only occur if someone was
incorrectly attempting to register a new suite via update.
> > + __func__, updated_sinfo->pName);
> > + return -1;
> > + }
> > +
> > + /* lookup the associated CUnit suite */
> > + suite = CU_get_suite_by_name(sinfo->pName, CU_get_registry());
> > + if (!suite) {
> > + fprintf(stderr, "%s: can't find registered suite %s\n",
>
> shouldn't it say "unable to find existing suite with matching name in
> registery"?
OK. This means I'll have to figure out what our policy is on long vs
wrapped lines as I've lost track :)
> > + __func__, sinfo->pName);
> > + return CU_get_error();
> > + }
> > +
> > + sinfo->pInitFunc = updated_sinfo->pInitFunc;
> > + sinfo->pCleanupFunc = updated_sinfo->pCleanupFunc;
> > +
> > + CU_set_suite_cleanupfunc(suite, updated_sinfo->pCleanupFunc);
> > +
> > + for (tinfo = updated_sinfo->pTests; tinfo->testinfo.pName; tinfo++) {
> > + test = CU_get_test(suite, tinfo->testinfo.pName);
> > +
> > + if (test) {
> > + CU_ErrorCode err = CU_set_test_func(test,
> > + tinfo->testinfo.pTestFunc);
> > + if (err != CUE_SUCCESS)
> > + return -1;
> > +
> > + if (cunit_update_test(sinfo, tinfo) != 0)
> > + return -1;
> > + } else {
> > + fprintf(stderr, "%s: unable to add new test: %s\n",
> > + __func__, tinfo->testinfo.pName);
> > + return -1;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
>
> I am not sure about the meaning of the two above fuctions...
> From their names I understand that they should update the data associated
> with the tests, but the test function pointers contained in odp_suiteinfo_t
> don't seem to be taken into account...
> It looks like cunit_update_test() only updates the activity flag in the test,
> whereas cunit_update_suite() upddates init + cleanup + activity flag
> It would make sense to me to update the test function pointers as well in
> cunit_update_test().
> If not their name should indidate what they are intended to do.
>
As above, it does update the test function. To replace the test function
with a locally defined one you'd need the test name to match the
previously registered one (as this is the "official" name of the test
case being replaced) but the symbol name to be different (to avoid
potential collisions). The ODP_TEST_INFO* macros don't support this
but you could currently do it manually;
static odp_testinfo_t foo_tests_updates[] = {
{{"foo_test_b", my_local_foo_test_b}, NULL},
ODP_TEST_INFO_NULL
};
I think that's fine, given that I expect this is an uncommon use case.
--
Stuart.
_______________________________________________
lng-odp mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/lng-odp
