| Message ID | 1d2d13556ddbbbbb13989cdcb08447909bc4afaf.1415735831.git.shuahkh@osg.samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Sorry, I still really don't like this approach. While it is all in one place (thank you for that), I think it isn't a form that is very workable for the people maintaining the self tests. How about this, instead of per-Makefile customization, why not define an execution framework for these tests instead. For example, how about every test directory must have a Makefile with the needed binary targets. A common makefile could be included that defines the "run_tests" target that calls the script "run_tests.sh" that is a shell script in the current directory. (For inspiration, see how kernel modules can be built out of tree.) The "run_tests.sh" scripts could all include a common shell script, say "../common.sh" that provides any common variables, functions, etc (e.g. things like "Start $name test ..." should be in common.sh instead of repeated in every script, the installation logic can be in once place instead of repeated). Then along side common.sh could be "run_installed_tests.sh" or something, used on the installed target, that would traverse each directory, etc. From this, we can have a much more data-driven framework, and a common approach to running tests. As such, we should declare up front how tests should behave on failure. And the top-level test runner can do things like count the number of tests, failures, etc. Then, instead of splitting up the patches by test directory, you can split them up by logical changes (e.g. defining the common "run_tests" target, and then removing the target from all the tests by including the common makefile stub that defines it). -Kees On Tue, Nov 11, 2014 at 12:27 PM, Shuah Khan <shuahkh@osg.samsung.com> wrote: > Add a new make target to enable installing test. This target > installs test in the kselftest install location and add to the > kselftest script to run the test. Install target can be run > only from top level source dir. > > Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > --- > tools/testing/selftests/firmware/Makefile | 49 ++++++++++++++++++++----------- > 1 file changed, 32 insertions(+), 17 deletions(-) > > diff --git a/tools/testing/selftests/firmware/Makefile b/tools/testing/selftests/firmware/Makefile > index e23cce0..2286dbb 100644 > --- a/tools/testing/selftests/firmware/Makefile > +++ b/tools/testing/selftests/firmware/Makefile > @@ -1,25 +1,40 @@ > # Makefile for firmware loading selftests > > # No binaries, but make sure arg-less "make" doesn't trigger "run_tests" > + > +__fw_filesystem: > +fw_filesystem = if /bin/sh ./fw_filesystem.sh ; then > +fw_filesystem += echo fw_filesystem: ok; > +fw_filesystem += else echo fw_filesystem: [FAIL]; > +fw_filesystem += fi > + > +__fw_userhelper: > +fw_userhelper = if /bin/sh ./fw_userhelper.sh ; then > +fw_userhelper += echo fw_userhelper: ok; > +fw_userhelper += else > +fw_userhelper += echo fw_userhelper: [FAIL]; > +fw_userhelper += fi > + > all: > > -fw_filesystem: > - @if /bin/sh ./fw_filesystem.sh ; then \ > - echo "fw_filesystem: ok"; \ > - else \ > - echo "fw_filesystem: [FAIL]"; \ > - exit 1; \ > - fi > - > -fw_userhelper: > - @if /bin/sh ./fw_userhelper.sh ; then \ > - echo "fw_userhelper: ok"; \ > - else \ > - echo "fw_userhelper: [FAIL]"; \ > - exit 1; \ > - fi > - > -run_tests: all fw_filesystem fw_userhelper > +install: > +ifdef INSTALL_KSFT_PATH > + install ./fw_filesystem.sh ./fw_userhelper.sh $(INSTALL_KSFT_PATH) > + @echo echo Start firmware filesystem test .... >> $(KSELFTEST) > + @echo "$(fw_filesystem)" >> $(KSELFTEST) > + @echo echo End firmware filesystem test .... >> $(KSELFTEST) > + @echo echo -------------------- >> $(KSELFTEST) > + @echo echo Start firmware userhelper test .... >> $(KSELFTEST) > + @echo "$(fw_userhelper)" >> $(KSELFTEST) > + @echo echo End firmware userhelper test .... >> $(KSELFTEST) > + @echo echo ============================== >> $(KSELFTEST) > +else > + @echo Run make kselftest_install in top level source directory > +endif > + > +run_tests: > + @$(fw_filesystem) > + @$(fw_userhelper) > > # Nothing to clean up. > clean: > -- > 1.9.1 >
On 11/11/2014 02:29 PM, Kees Cook wrote: > Hi, > > Sorry, I still really don't like this approach. While it is all in one > place (thank you for that), I think it isn't a form that is very > workable for the people maintaining the self tests. How about this, > instead of per-Makefile customization, why not define an execution > framework for these tests instead. If I understand correctly, sounds like you don't like the way install target is implemented in the individual test Makefiles and the changes I made to run_tests targets to address the code duplication concern. At the moment there is no duplicate code in this patch series between install and run_tests targets. This is a first step towards standardizing the framework and a definite improvement over what we have currently. As I mentioned earlier, my goal is to make it easier for developers to install and run the existing tests and evolve the framework as we go. Assuming my understanding is correct that: -- install and run_tests targets in individual tests can be refined and automated with a common Makefile approach you proposed. -- the rest of the user-interface kselftest_install and kselftest are good. I would like to propose that we get started with the current implementation and refine it based on the following ideas you suggested. The refinements you are recommending are confined to selftests and can be made after the kselftest_install gets added. Adding kselftest_install makes it easier to make the refinements as it defines overall UI. > > For example, how about every test directory must have a Makefile with > the needed binary targets. A common makefile could be included that > defines the "run_tests" target that calls the script "run_tests.sh" > that is a shell script in the current directory. (For inspiration, see > how kernel modules can be built out of tree.) > > The "run_tests.sh" scripts could all include a common shell script, > say "../common.sh" that provides any common variables, functions, etc > (e.g. things like "Start $name test ..." should be in common.sh > instead of repeated in every script, the installation logic can be in > once place instead of repeated). > > Then along side common.sh could be "run_installed_tests.sh" or > something, used on the installed target, that would traverse each > directory, etc. From this, we can have a much more data-driven > framework, and a common approach to running tests. > > As such, we should declare up front how tests should behave on > failure. And the top-level test runner can do things like count the > number of tests, failures, etc. > > Then, instead of splitting up the patches by test directory, you can > split them up by logical changes (e.g. defining the common "run_tests" > target, and then removing the target from all the tests by including > the common makefile stub that defines it). > These are good ideas and I am with on evolving the framework to make it easier to maintain individual tests. Patches are welcome. thanks, -- Shuah
On 11/11/2014 06:06 PM, Shuah Khan wrote: > On 11/11/2014 02:29 PM, Kees Cook wrote: >> Hi, >> >> Sorry, I still really don't like this approach. While it is all in one >> place (thank you for that), I think it isn't a form that is very >> workable for the people maintaining the self tests. How about this, >> instead of per-Makefile customization, why not define an execution >> framework for these tests instead. > > If I understand correctly, sounds like you don't like the way > install target is implemented in the individual test Makefiles > and the changes I made to run_tests targets to address the code > duplication concern. > > At the moment there is no duplicate code in this patch series > between install and run_tests targets. This is a first step > towards standardizing the framework and a definite improvement > over what we have currently. As I mentioned earlier, my goal > is to make it easier for developers to install and run the > existing tests and evolve the framework as we go. > > Assuming my understanding is correct that: > > -- install and run_tests targets in individual tests can be > refined and automated with a common Makefile approach you > proposed. > -- the rest of the user-interface kselftest_install and kselftest > are good. > > I would like to propose that we get started with the current > implementation and refine it based on the following ideas you > suggested. The refinements you are recommending are confined > to selftests and can be made after the kselftest_install > gets added. Adding kselftest_install makes it easier to make > the refinements as it defines overall UI. > Hi Kees, Are you ok with the above proposal? I understand this approach might not be perfect, however it is a step in the right direction to enable and make it easier to run them. I would like to get some initial work in for 3.19 if at all possible. I plan to evolve the back-end to make it easier to write and maintain for developers writing tests going forward. thanks, -- Shuah
diff --git a/tools/testing/selftests/firmware/Makefile b/tools/testing/selftests/firmware/Makefile index e23cce0..2286dbb 100644 --- a/tools/testing/selftests/firmware/Makefile +++ b/tools/testing/selftests/firmware/Makefile @@ -1,25 +1,40 @@ # Makefile for firmware loading selftests # No binaries, but make sure arg-less "make" doesn't trigger "run_tests" + +__fw_filesystem: +fw_filesystem = if /bin/sh ./fw_filesystem.sh ; then +fw_filesystem += echo fw_filesystem: ok; +fw_filesystem += else echo fw_filesystem: [FAIL]; +fw_filesystem += fi + +__fw_userhelper: +fw_userhelper = if /bin/sh ./fw_userhelper.sh ; then +fw_userhelper += echo fw_userhelper: ok; +fw_userhelper += else +fw_userhelper += echo fw_userhelper: [FAIL]; +fw_userhelper += fi + all: -fw_filesystem: - @if /bin/sh ./fw_filesystem.sh ; then \ - echo "fw_filesystem: ok"; \ - else \ - echo "fw_filesystem: [FAIL]"; \ - exit 1; \ - fi - -fw_userhelper: - @if /bin/sh ./fw_userhelper.sh ; then \ - echo "fw_userhelper: ok"; \ - else \ - echo "fw_userhelper: [FAIL]"; \ - exit 1; \ - fi - -run_tests: all fw_filesystem fw_userhelper +install: +ifdef INSTALL_KSFT_PATH + install ./fw_filesystem.sh ./fw_userhelper.sh $(INSTALL_KSFT_PATH) + @echo echo Start firmware filesystem test .... >> $(KSELFTEST) + @echo "$(fw_filesystem)" >> $(KSELFTEST) + @echo echo End firmware filesystem test .... >> $(KSELFTEST) + @echo echo -------------------- >> $(KSELFTEST) + @echo echo Start firmware userhelper test .... >> $(KSELFTEST) + @echo "$(fw_userhelper)" >> $(KSELFTEST) + @echo echo End firmware userhelper test .... >> $(KSELFTEST) + @echo echo ============================== >> $(KSELFTEST) +else + @echo Run make kselftest_install in top level source directory +endif + +run_tests: + @$(fw_filesystem) + @$(fw_userhelper) # Nothing to clean up. clean:
Add a new make target to enable installing test. This target installs test in the kselftest install location and add to the kselftest script to run the test. Install target can be run only from top level source dir. Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> --- tools/testing/selftests/firmware/Makefile | 49 ++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 17 deletions(-)