On 16 September 2015 at 18:07, Stuart Haslam <[email protected]> wrote:
> 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. > Yes please... I understood from the code, but I felt the code did not match the function name. Why not calling it ...update_properties? > > > > + > > > +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. > Then maybe it should say "text XXX not found: use YYY to add new tests"... > > > > + __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 :) > I won't fight you with the wording... > > > > + __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. > I think it is good enough too. As long as it is clear Christophe > > -- > Stuart. >
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
