| Message ID | CAO2Wz7ejapcVexoXWA-Db7Fot5m1Lrgf27xZHT+APf0a-Yj1Cw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 12 July 2016 at 09:44, Yi He <yi.he@linaro.org> wrote: > Hi, Christophe, > > This patch by its own misses one line change which will fail in 'make > check': > > diff --git a/test/all-platforms/performance/odp_l2fwd_run.sh > b/test/all-platforms/performance/odp_l2fwd_run.sh > index a33bbeb..2f390e4 100755 > --- a/test/all-platforms/performance/odp_l2fwd_run.sh > +++ b/test/all-platforms/performance/odp_l2fwd_run.sh > @@ -25,7 +25,7 @@ TEST_DIR="${TEST_DIR:-$PWD}" > # directory where test sources are, including scripts > TEST_SRC_DIR=$(dirname $0) > > -PATH=$TEST_DIR:$TEST_DIR/../../example/generator:$PATH > +PATH=$TEST_DIR:$TEST_DIR/../../../example/generator:$PATH Thanks for catching that: Addressed in V2 > > and some comments below: > > On 11 July 2016 at 23:26, Christophe Milard <christophe.milard@linaro.org> > wrote: >> >> API tests are now moved to test/all-platforms/validation/api >> (from test/validation), >> The reason for this move is two folded: >> * Moving down validation to all-plaform/validation disambiguates > > > all-platforms - minor but just in case the submit message. > meaning - in below? => v2 > >> the meanning of validation (which up to now was referring to both >> platform agnostic tests and to the set of tests to pass to be ODP >> compatible). Now things in test/all-platforms/ are platform agnostic. >> So test/all-platforms/validation/* are platform agnostic things for the >> valitation tests, as much as test/all-platforms/performance are > > > validation. =>V2 > >> platform agnostic things for the performance tests. >> * creating the api directory under "validation" simply enable adding >> other interfaces (such as future drv) as part of the validation tests >> >> Signed-off-by: Christophe Milard <christophe.milard@linaro.org> >> --- >> ... >> 161 files changed, 91 insertions(+), 75 deletions(-) >> create mode 100644 test/all-platforms/Makefile.am >> rename test/{validation => all-platforms}/common/Makefile.am (100%) >> rename test/{validation => all-platforms}/common/mask_common.c (100%) >> rename test/{validation => all-platforms}/common/mask_common.h (100%) >> rename test/{validation => all-platforms}/common/odp_cunit_common.c >> (100%) >> rename test/{validation => all-platforms}/common/odp_cunit_common.h >> (100%) > > > 'common' in validation actually holds the unit testing framework (cunit) > encapsulation > which would be better utilized by all test programs, what do you think? > currently not all test programs utilize the testing framework, is that > deliberately? Either I don't understand what you wrote (I apologize if so), or you missed a bit of this patch: If you look closely at the patch you will notice that it does create a subdirectory called "validation" and that all validation tests are moved in it, but you will also notice that the "common" directory (containig the c-unit stuff) is *not* moved there, hence achieving exactely what I think you described here: The c_unit "common" dir is then directely located under all-platforms hence giving the possibility to all tests (not only validation) to use c_unit. The platform specific ring tests actually use c_unit already (and are not part of the validation). So, I think this patch does actually do what you wanted :-), unless I misunderstood your comment. There is a little unpleasant thing remaining though: The common directory also contains a lib for the mask tests which I think should be pushed down one step down (under validation). In this patch, it is left with the c_unit stuff. That change can be done at a later stage. Whether this "common" directory (containing cunit stuff) should be called "framework" or something else, I don't know. It feels there are as many opinions as there are developers when it comes to naming things. But that could be another patch as well. I kept the name from before. Bill has scheduled a discussion on that patch on his arch call this afternoon. If is not too late for you, you are welcome to join. > > Shall this 'common' module to be lifted into top level and named like: > 'test/framework/cunit' > and can also put that lonely test_debug.h header into it since it looks like > belongs to a testing > framework. yes. would make sense to me, I think. still another patch > > This can be a separate item for future also. latterly better testing > framework can be evaluated > for needed features like multi-threading and mocker support. Thanks for your review, Christophe. > >> ...... >> diff --git a/test/all-platforms/Makefile.am >> b/test/all-platforms/Makefile.am >> new file mode 100644 >> index 0000000..af78bb6 >> --- /dev/null >> +++ b/test/all-platforms/Makefile.am >> @@ -0,0 +1,7 @@ >> +SUBDIRS = >> + >> +if cunit_support >> +SUBDIRS += common >> >> +endif > > > +SUBDIRS += common validation? > > For that validation need to be with cunit enabled > Also is that possible to write as below to eliminate one level Makefile.am?? > Then all-platforms/validation/Makefile.am can be removed. > +SUBDIRES += common validation/api I have struggled a lot with autotools here, mostly due to the fact that the perf test needs pktio env (i.e. the platform side needs the "all-platforms" side (which is ok), but sadly, today, we have dependency on the other direction as well. My suggestion is to keep this as it works, then clean up the pktio_env mess and push running perf tests from platform side. Then we could simplify that better. Unless I misunderstood you point. Christophe > >> >> + >> +SUBDIRS += performance miscellaneous validation > > > +SUBDIRS += performance miscellaneous >
> > > 'common' in validation actually holds the unit testing framework (cunit) > > encapsulation > > which would be better utilized by all test programs, what do you think? > > currently not all test programs utilize the testing framework, is that > > deliberately? > > Either I don't understand what you wrote (I apologize if so), or you > missed a bit of this patch: If you look closely at the patch you will > notice that it does create a subdirectory called "validation" and that > all validation tests are moved in it, but you will also notice that > the "common" directory (containig the c-unit stuff) is *not* moved > there, hence achieving exactely what I think you described here: The > c_unit "common" dir is then directely located under all-platforms > hence giving the possibility to all tests (not only validation) to use > c_unit. The platform specific ring tests actually use c_unit already > (and are not part of the validation). > So, I think this patch does actually do what you wanted :-), unless I > misunderstood your comment. > There is a little unpleasant thing remaining though: The common > directory also contains a lib for the mask tests which I think should > be pushed down one step down (under validation). In this patch, it is > left with the c_unit stuff. That change can be done at a later stage. > Whether this "common" directory (containing cunit stuff) should be > called "framework" or something else, I don't know. It feels there are > as many opinions as there are developers when it comes to naming > things. But that could be another patch as well. I kept the name from > before. > Bill has scheduled a discussion on that patch on his arch call this > afternoon. If is not too late for you, you are welcome to join. > > > > Shall this 'common' module to be lifted into top level and named like: > > 'test/framework/cunit' > > and can also put that lonely test_debug.h header into it since it looks > like > > belongs to a testing > > framework. > > yes. would make sense to me, I think. still another patch > Yes, should raise it separately, leave it to separate discussion. what I mean is renaming 'common' to more reflecting name (as testing framework encapsulation, 'test/framework/cunit') and put it in proper position to be used by all test programs. > > >> diff --git a/test/all-platforms/Makefile.am > >> b/test/all-platforms/Makefile.am > >> new file mode 100644 > >> index 0000000..af78bb6 > >> --- /dev/null > >> +++ b/test/all-platforms/Makefile.am > >> @@ -0,0 +1,7 @@ > >> +SUBDIRS > > I have struggled a lot with autotools here, mostly due to the fact > that the perf test needs pktio env (i.e. the platform side needs the > "all-platforms" side (which is ok), but sadly, today, we have > dependency on the other direction as well. My suggestion is to keep > this as it works, then clean up the pktio_env mess and push running > perf tests from platform side. Then we could simplify that better. > Unless I misunderstood you point. > OK, understand this.
diff --git a/test/all-platforms/performance/odp_l2fwd_run.sh b/test/all-platforms/performance/odp_l2fwd_run.sh index a33bbeb..2f390e4 100755 --- a/test/all-platforms/performance/odp_l2fwd_run.sh +++ b/test/all-platforms/performance/odp_l2fwd_run.sh @@ -25,7 +25,7 @@ TEST_DIR="${TEST_DIR:-$PWD}" # directory where test sources are, including scripts TEST_SRC_DIR=$(dirname $0) -PATH=$TEST_DIR:$TEST_DIR/../../example/generator:$PATH +PATH=$TEST_DIR:$TEST_DIR/../../../example/generator:$PATH and some comments below: