* [Buildroot] [RFC v1 0/1] Add external test support
@ 2023-12-22 23:22 Colin Foster
2023-12-22 23:22 ` [Buildroot] [RFC v1 1/1] support/testing/run-tests: add ability to run tests from external Colin Foster
0 siblings, 1 reply; 6+ messages in thread
From: Colin Foster @ 2023-12-22 23:22 UTC (permalink / raw)
To: buildroot; +Cc: Ricardo Martincoski
NOTE: This is an RFC that isn't fully functional! Do not merge!
Add the ability to create and maintain tests in a BR2_EXTERNAL
directory.
I recently submitted a simple tftpy test module. My current system is
based on Buildroot 2023.02.x, and I would like to be able to run
verifications without modification to Buildroot and without waiting
until 2024.02.x lands.
There were some Python dependencies that led me to this current
iteration, which is to create a temporary directory for all tests and
pointing nose2 to the temp directory. I'm not sure if that is elegant,
or kludgy... so I'm looking for feedback on that.
I also tested this with my "tftpy" script, but it currently fails.
```
(.venv) $ BR2_EXTERNAL=`pwd`/buildroot-external ./buildroot/support/testing/run-tests -d dl -o output_folder -k tests.package.test_python_tftpy.TestPythonTftpy
17:07:56 TestPythonTftpy Starting
17:07:56 TestPythonTftpy Building
E
======================================================================
ERROR: test_run (tests.package.test_python_tftpy.TestPythonTftpy)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/path/to/pwd/buildroot/support/testing/infra/basetest.py", line 73, in setUp
self.b.build()
File "/path/to/pwd/buildroot/support/testing/infra/builder.py", line 98, in build
raise SystemError("Build failed")
SystemError: Build failed
----------------------------------------------------------------------
Ran 1 test in 4.925s
FAILED (errors=1)
```
I figure I should get some feedback on my approach before digging into
this too much further. I did run a simple verification that the
functional tests aren't broken:
```
(.venv) colin@colin-ia-desktop:ia_buildroot_overlay$ BR2_EXTERNAL=`pwd`/buildroot-external ./buildroot/support/testing/run-tests -d dl -o output
_folder -k tests.package.test_python_aexpect.TestPythonPy3Aexpect
17:12:55 TestPythonPy3Aexpect Starting
17:13:01 TestPythonPy3Aexpect Cleaning up
.
----------------------------------------------------------------------
Ran 1 test in 6.521s
OK
```
Thoughts on this approach?
Is there a different way to overlay the external directory that I'm
overlooking?
If this approach is valid... then why might the test be failing? I'm
surprised to see "/path/to/pwd/buildroot/support/testing/infra/*"
referenced in the output... I'd expect it to be
"/tmp/tmp*/testing/infra/*".
Thanks for any feedback!
Colin Foster (1):
support/testing/run-tests: add ability to run tests from external
support/testing/run-tests | 163 +++++++++++++++++++++-----------------
1 file changed, 91 insertions(+), 72 deletions(-)
--
2.34.1
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot
^ permalink raw reply [flat|nested] 6+ messages in thread* [Buildroot] [RFC v1 1/1] support/testing/run-tests: add ability to run tests from external 2023-12-22 23:22 [Buildroot] [RFC v1 0/1] Add external test support Colin Foster @ 2023-12-22 23:22 ` Colin Foster 2023-12-23 13:40 ` Thomas Petazzoni via buildroot 2023-12-23 13:54 ` Yann E. MORIN 0 siblings, 2 replies; 6+ messages in thread From: Colin Foster @ 2023-12-22 23:22 UTC (permalink / raw) To: buildroot; +Cc: Ricardo Martincoski The BR2_EXTERNAL variable can be used to keep customizations outside of Buildroot. There was no support for writing tests in those external environments. When the BR2_EXTERNAL environment variable is used for running tests, include the tests found in ${BR2_EXTERNAL}/support/testing. Signed-off-by: Colin Foster <colin.foster@in-advantage.com> --- support/testing/run-tests | 163 +++++++++++++++++++++----------------- 1 file changed, 91 insertions(+), 72 deletions(-) diff --git a/support/testing/run-tests b/support/testing/run-tests index 485811b746..9ed689e177 100755 --- a/support/testing/run-tests +++ b/support/testing/run-tests @@ -3,6 +3,8 @@ import argparse import multiprocessing import os import sys +import tempfile +import shutil import nose2 @@ -40,95 +42,112 @@ def main(): args = parser.parse_args() script_path = os.path.realpath(__file__) - test_dir = os.path.dirname(script_path) - - if args.stdout: - BRConfigTest.logtofile = False - - if args.list: - print("List of tests") - nose2.discover(argv=[script_path, - "-s", test_dir, - "-v", - "--collect-only"], - plugins=["nose2.plugins.collect"]) - return 0 - - if args.download is None: - args.download = os.getenv("BR2_DL_DIR") + script_dir = os.path.dirname(script_path) + + overlay_path = os.getenv("BR2_EXTERNAL") + if overlay_path: + temp_dir = tempfile.TemporaryDirectory() + shutil.copytree(script_dir, temp_dir.name, dirs_exist_ok=True) + shutil.copytree(overlay_path + "/support/testing", + temp_dir.name, + dirs_exist_ok=True) + test_dir = temp_dir.name + else: + test_dir = script_dir + + try: + if args.stdout: + BRConfigTest.logtofile = False + + if args.list: + print("List of tests") + nose2.discover(argv=[script_path, + "-s", test_dir, + "-v", + "--collect-only"], + plugins=["nose2.plugins.collect"]) + return 0 + if args.download is None: - print("Missing download directory, please use -d/--download") + args.download = os.getenv("BR2_DL_DIR") + if args.download is None: + print("Missing download directory, please use -d/--download") + print("") + parser.print_help() + return 1 + + BRConfigTest.downloaddir = os.path.abspath(args.download) + + if args.prepare_only: + emulator_builtin_binaries = ["kernel-vexpress-5.10.202", + "vexpress-v2p-ca9-5.10.202.dtb", + "kernel-versatile-5.10.202", + "versatile-pb-5.10.202.dtb"] + print("Downloading emulator builtin binaries") + for binary in emulator_builtin_binaries: + infra.download(BRConfigTest.downloaddir, binary) + return 0 + + if args.output is None: + print("Missing output directory, please use -o/--output") print("") parser.print_help() return 1 - BRConfigTest.downloaddir = os.path.abspath(args.download) - - if args.prepare_only: - emulator_builtin_binaries = ["kernel-vexpress-5.10.202", - "vexpress-v2p-ca9-5.10.202.dtb", - "kernel-versatile-5.10.202", - "versatile-pb-5.10.202.dtb"] - print("Downloading emulator builtin binaries") - for binary in emulator_builtin_binaries: - infra.download(BRConfigTest.downloaddir, binary) - return 0 - - if args.output is None: - print("Missing output directory, please use -o/--output") - print("") - parser.print_help() - return 1 - - if not os.path.exists(args.output): - os.mkdir(args.output) + if not os.path.exists(args.output): + os.mkdir(args.output) - BRConfigTest.outputdir = os.path.abspath(args.output) + BRConfigTest.outputdir = os.path.abspath(args.output) - if args.all is False and not args.testname: - print("No test selected") - print("") - parser.print_help() - return 1 - - BRConfigTest.keepbuilds = args.keep - - if args.testcases != 1: - if args.testcases < 1: - print("Invalid number of testcases to run simultaneously") + if args.all is False and not args.testname: + print("No test selected") print("") parser.print_help() return 1 - # same default BR2_JLEVEL as package/Makefile.in - br2_jlevel = 1 + multiprocessing.cpu_count() - each_testcase = int((br2_jlevel + args.testcases) / args.testcases) - BRConfigTest.jlevel = each_testcase - - if args.jlevel: - if args.jlevel < 0: - print("Invalid BR2_JLEVEL to use for each testcase") + + BRConfigTest.keepbuilds = args.keep + + if args.testcases != 1: + if args.testcases < 1: + print("Invalid number of testcases to run simultaneously") + print("") + parser.print_help() + return 1 + # same default BR2_JLEVEL as package/Makefile.in + br2_jlevel = 1 + multiprocessing.cpu_count() + each_testcase = int((br2_jlevel + args.testcases) / args.testcases) + BRConfigTest.jlevel = each_testcase + + if args.jlevel: + if args.jlevel < 0: + print("Invalid BR2_JLEVEL to use for each testcase") + print("") + parser.print_help() + return 1 + # the user can override the auto calculated value + BRConfigTest.jlevel = args.jlevel + + if args.timeout_multiplier < 1: + print("Invalid multiplier for timeout values") print("") parser.print_help() return 1 - # the user can override the auto calculated value - BRConfigTest.jlevel = args.jlevel + BRConfigTest.timeout_multiplier = args.timeout_multiplier + + nose2_args = ["-v", + "-N", str(args.testcases), + "-s", test_dir, + "-c", os.path.join(test_dir, "conf/unittest.cfg")] - if args.timeout_multiplier < 1: - print("Invalid multiplier for timeout values") - print("") - parser.print_help() - return 1 - BRConfigTest.timeout_multiplier = args.timeout_multiplier + if args.testname: + nose2_args += args.testname - nose2_args = ["-v", - "-N", str(args.testcases), - "-s", test_dir, - "-c", os.path.join(test_dir, "conf/unittest.cfg")] + nose2.discover(argv=nose2_args) - if args.testname: - nose2_args += args.testname + finally: + if temp_dir: + shutil.rmtree(temp_dir.name) - nose2.discover(argv=nose2_args) if __name__ == "__main__": -- 2.34.1 _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Buildroot] [RFC v1 1/1] support/testing/run-tests: add ability to run tests from external 2023-12-22 23:22 ` [Buildroot] [RFC v1 1/1] support/testing/run-tests: add ability to run tests from external Colin Foster @ 2023-12-23 13:40 ` Thomas Petazzoni via buildroot 2023-12-23 16:12 ` Colin Foster 2023-12-23 13:54 ` Yann E. MORIN 1 sibling, 1 reply; 6+ messages in thread From: Thomas Petazzoni via buildroot @ 2023-12-23 13:40 UTC (permalink / raw) To: Colin Foster; +Cc: Ricardo Martincoski, buildroot Hello Colin, On Fri, 22 Dec 2023 17:22:51 -0600 Colin Foster <colin.foster@in-advantage.com> wrote: > + overlay_path = os.getenv("BR2_EXTERNAL") > + if overlay_path: > + temp_dir = tempfile.TemporaryDirectory() > + shutil.copytree(script_dir, temp_dir.name, dirs_exist_ok=True) > + shutil.copytree(overlay_path + "/support/testing", > + temp_dir.name, > + dirs_exist_ok=True) Is there a better solution than copying all tests into a temporary directory? Thomas -- Thomas Petazzoni, co-owner and CEO, Bootlin Embedded Linux and Kernel engineering and training https://bootlin.com _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Buildroot] [RFC v1 1/1] support/testing/run-tests: add ability to run tests from external 2023-12-23 13:40 ` Thomas Petazzoni via buildroot @ 2023-12-23 16:12 ` Colin Foster 0 siblings, 0 replies; 6+ messages in thread From: Colin Foster @ 2023-12-23 16:12 UTC (permalink / raw) To: Thomas Petazzoni; +Cc: Ricardo Martincoski, buildroot Hi Thomas, On Sat, Dec 23, 2023 at 02:40:18PM +0100, Thomas Petazzoni wrote: > Hello Colin, > > On Fri, 22 Dec 2023 17:22:51 -0600 > Colin Foster <colin.foster@in-advantage.com> wrote: > > > + overlay_path = os.getenv("BR2_EXTERNAL") > > + if overlay_path: > > + temp_dir = tempfile.TemporaryDirectory() > > + shutil.copytree(script_dir, temp_dir.name, dirs_exist_ok=True) > > + shutil.copytree(overlay_path + "/support/testing", > > + temp_dir.name, > > + dirs_exist_ok=True) > > Is there a better solution than copying all tests into a temporary > directory? I hope so. Yann has suggestions for using symlinks that would probably be better if it works. > > Thomas > -- > Thomas Petazzoni, co-owner and CEO, Bootlin > Embedded Linux and Kernel engineering and training > https://bootlin.com _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Buildroot] [RFC v1 1/1] support/testing/run-tests: add ability to run tests from external 2023-12-22 23:22 ` [Buildroot] [RFC v1 1/1] support/testing/run-tests: add ability to run tests from external Colin Foster 2023-12-23 13:40 ` Thomas Petazzoni via buildroot @ 2023-12-23 13:54 ` Yann E. MORIN 2023-12-23 17:18 ` Colin Foster 1 sibling, 1 reply; 6+ messages in thread From: Yann E. MORIN @ 2023-12-23 13:54 UTC (permalink / raw) To: Colin Foster; +Cc: Ricardo Martincoski, buildroot Colin, All, Thanks for this RFC patch! See below a few preliminary comments... On 2023-12-22 17:22 -0600, Colin Foster spake thusly: > The BR2_EXTERNAL variable can be used to keep customizations outside of > Buildroot. There was no support for writing tests in those external > environments. > > When the BR2_EXTERNAL environment variable is used for running tests, > include the tests found in ${BR2_EXTERNAL}/support/testing. I'm a bit conflicted about that situation, to be honest... On one hand, I do understand the motivation for trying and reusing the Buildroot infrastructure for one's own tests; that's great and means that what we have is actually useful to some people. :-) On the other hand, I feel like whatever lies in an external tree is custom packages for nternal use by a company, and so in such cases, the testing infra would be better served by having CI running ad-hoc tests. What I mean is that the runtime testing infra in Buildroot is meant as a way to validate that the package is, at the very least, properly integrated in Buildroot, e.g. that the runtime dependencies are accounted for; it is not meant as a generic runtime infrastructure, while what companies (those most likely to be using br2-external) want is actual testing, with full functionality and non-regression suites and what-have-you. Also, the rationale you provided, testing the tftpy package, is not appropriate here: tftpy is in Buildrooot, so its runtime test should also be there, so writing a runtime test in a br2-external does not make sense in this specific case. Of course, I said I was conflicted about this: I have no strong opinion against the feature itself; I just don't feel in favour either... Other maintainers will have to weight in. However, there is a case for looking the code, so let's go there. First, don't forget that BR2_EXTERNAL is a space-separated list of directories. Your current patch only considers a single directory will be used, which is generally not true. Second, and most importantly, I am not too happy about the need to do some copying around. It is very often that I hit Ctrl-C to quickly kill a set of runtime tests, and that would not be caught by the try-finally clause (the first Ctrl-C would, not the second). So that could leave quite a lot of directories around; for long-running machines like build-farm servers, this is going to be a problem... Also, tempfile.TemporaryDirectory() uses the same rules as mkdtemp(), which uses the same rules as mkstemp(), to decide where to create the temporary directory. On many machine, that resorts to using on of the TMPDIR, TEMP or TMP environment variables, and it is not unusual for those to point to .tmp, which in turns is very often a tmpfs, so does not get a lot of space in there; even if only the infra scripts are copied, ad those are not so huge, it is still not very nice to leave around... So, the question is: why do we need to copy? I guess the reason is that nose2 can only have one starting directory where it looks for tests, right? A few years ago, I was working on moving the packages' runtime tests to the respective package directory: https://gitlab.com/ymorin/buildroot/-/tree/yem/package-tests https://gitlab.com/ymorin/buildroot/-/commits/yem/package-tests Of particular note, I came up with using symlinks, so that the packages tests would be eventually discovered by nose2; https://gitlab.com/ymorin/buildroot/-/commit/1d9fc876483727c77ec93b4657cd5b4884e2adbb#a66a9b8b0912024dc207cec14b32e038e0c208eb Basically, what that would do, is scan the package directory for files named 'PACKAGE_NAME.py' and create a symlink test_PACKAGE_NAME.py in the runtime test infra. I wonder if that would not be a better solution... I have also not looked in details at the patch, because the change in indentation due to the try-finally, is difficult to follow. If we are to go with a solution similar to what you propose, then it would be nice to have the patch split in at least three patches: - one to introduce the try-finally, without any other change - one to do the copy of the infra unconditionally - one to copy each of the br2-external trees' test suites But please don't respin too fast, let the others look and chime with their opinions and reviews... Regards, Yann E. MORIN. > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > --- > support/testing/run-tests | 163 +++++++++++++++++++++----------------- > 1 file changed, 91 insertions(+), 72 deletions(-) > > diff --git a/support/testing/run-tests b/support/testing/run-tests > index 485811b746..9ed689e177 100755 > --- a/support/testing/run-tests > +++ b/support/testing/run-tests > @@ -3,6 +3,8 @@ import argparse > import multiprocessing > import os > import sys > +import tempfile > +import shutil > > import nose2 > > @@ -40,95 +42,112 @@ def main(): > args = parser.parse_args() > > script_path = os.path.realpath(__file__) > - test_dir = os.path.dirname(script_path) > - > - if args.stdout: > - BRConfigTest.logtofile = False > - > - if args.list: > - print("List of tests") > - nose2.discover(argv=[script_path, > - "-s", test_dir, > - "-v", > - "--collect-only"], > - plugins=["nose2.plugins.collect"]) > - return 0 > - > - if args.download is None: > - args.download = os.getenv("BR2_DL_DIR") > + script_dir = os.path.dirname(script_path) > + > + overlay_path = os.getenv("BR2_EXTERNAL") > + if overlay_path: > + temp_dir = tempfile.TemporaryDirectory() > + shutil.copytree(script_dir, temp_dir.name, dirs_exist_ok=True) > + shutil.copytree(overlay_path + "/support/testing", > + temp_dir.name, > + dirs_exist_ok=True) > + test_dir = temp_dir.name > + else: > + test_dir = script_dir > + > + try: > + if args.stdout: > + BRConfigTest.logtofile = False > + > + if args.list: > + print("List of tests") > + nose2.discover(argv=[script_path, > + "-s", test_dir, > + "-v", > + "--collect-only"], > + plugins=["nose2.plugins.collect"]) > + return 0 > + > if args.download is None: > - print("Missing download directory, please use -d/--download") > + args.download = os.getenv("BR2_DL_DIR") > + if args.download is None: > + print("Missing download directory, please use -d/--download") > + print("") > + parser.print_help() > + return 1 > + > + BRConfigTest.downloaddir = os.path.abspath(args.download) > + > + if args.prepare_only: > + emulator_builtin_binaries = ["kernel-vexpress-5.10.202", > + "vexpress-v2p-ca9-5.10.202.dtb", > + "kernel-versatile-5.10.202", > + "versatile-pb-5.10.202.dtb"] > + print("Downloading emulator builtin binaries") > + for binary in emulator_builtin_binaries: > + infra.download(BRConfigTest.downloaddir, binary) > + return 0 > + > + if args.output is None: > + print("Missing output directory, please use -o/--output") > print("") > parser.print_help() > return 1 > > - BRConfigTest.downloaddir = os.path.abspath(args.download) > - > - if args.prepare_only: > - emulator_builtin_binaries = ["kernel-vexpress-5.10.202", > - "vexpress-v2p-ca9-5.10.202.dtb", > - "kernel-versatile-5.10.202", > - "versatile-pb-5.10.202.dtb"] > - print("Downloading emulator builtin binaries") > - for binary in emulator_builtin_binaries: > - infra.download(BRConfigTest.downloaddir, binary) > - return 0 > - > - if args.output is None: > - print("Missing output directory, please use -o/--output") > - print("") > - parser.print_help() > - return 1 > - > - if not os.path.exists(args.output): > - os.mkdir(args.output) > + if not os.path.exists(args.output): > + os.mkdir(args.output) > > - BRConfigTest.outputdir = os.path.abspath(args.output) > + BRConfigTest.outputdir = os.path.abspath(args.output) > > - if args.all is False and not args.testname: > - print("No test selected") > - print("") > - parser.print_help() > - return 1 > - > - BRConfigTest.keepbuilds = args.keep > - > - if args.testcases != 1: > - if args.testcases < 1: > - print("Invalid number of testcases to run simultaneously") > + if args.all is False and not args.testname: > + print("No test selected") > print("") > parser.print_help() > return 1 > - # same default BR2_JLEVEL as package/Makefile.in > - br2_jlevel = 1 + multiprocessing.cpu_count() > - each_testcase = int((br2_jlevel + args.testcases) / args.testcases) > - BRConfigTest.jlevel = each_testcase > - > - if args.jlevel: > - if args.jlevel < 0: > - print("Invalid BR2_JLEVEL to use for each testcase") > + > + BRConfigTest.keepbuilds = args.keep > + > + if args.testcases != 1: > + if args.testcases < 1: > + print("Invalid number of testcases to run simultaneously") > + print("") > + parser.print_help() > + return 1 > + # same default BR2_JLEVEL as package/Makefile.in > + br2_jlevel = 1 + multiprocessing.cpu_count() > + each_testcase = int((br2_jlevel + args.testcases) / args.testcases) > + BRConfigTest.jlevel = each_testcase > + > + if args.jlevel: > + if args.jlevel < 0: > + print("Invalid BR2_JLEVEL to use for each testcase") > + print("") > + parser.print_help() > + return 1 > + # the user can override the auto calculated value > + BRConfigTest.jlevel = args.jlevel > + > + if args.timeout_multiplier < 1: > + print("Invalid multiplier for timeout values") > print("") > parser.print_help() > return 1 > - # the user can override the auto calculated value > - BRConfigTest.jlevel = args.jlevel > + BRConfigTest.timeout_multiplier = args.timeout_multiplier > + > + nose2_args = ["-v", > + "-N", str(args.testcases), > + "-s", test_dir, > + "-c", os.path.join(test_dir, "conf/unittest.cfg")] > > - if args.timeout_multiplier < 1: > - print("Invalid multiplier for timeout values") > - print("") > - parser.print_help() > - return 1 > - BRConfigTest.timeout_multiplier = args.timeout_multiplier > + if args.testname: > + nose2_args += args.testname > > - nose2_args = ["-v", > - "-N", str(args.testcases), > - "-s", test_dir, > - "-c", os.path.join(test_dir, "conf/unittest.cfg")] > + nose2.discover(argv=nose2_args) > > - if args.testname: > - nose2_args += args.testname > + finally: > + if temp_dir: > + shutil.rmtree(temp_dir.name) > > - nose2.discover(argv=nose2_args) > > > if __name__ == "__main__": > -- > 2.34.1 > > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot -- .-----------------.--------------------.------------------.--------------------. | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | '------------------------------^-------^------------------^--------------------' _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Buildroot] [RFC v1 1/1] support/testing/run-tests: add ability to run tests from external 2023-12-23 13:54 ` Yann E. MORIN @ 2023-12-23 17:18 ` Colin Foster 0 siblings, 0 replies; 6+ messages in thread From: Colin Foster @ 2023-12-23 17:18 UTC (permalink / raw) To: Yann E. MORIN; +Cc: Ricardo Martincoski, buildroot Hi Yann, Thanks for the feedback! On Sat, Dec 23, 2023 at 02:54:22PM +0100, Yann E. MORIN wrote: > Colin, All, > > Thanks for this RFC patch! See below a few preliminary comments... > ... > What I mean is that the runtime testing infra in Buildroot is meant as a > way to validate that the package is, at the very least, properly > integrated in Buildroot, e.g. that the runtime dependencies are > accounted for; it is not meant as a generic runtime infrastructure, > while what companies (those most likely to be using br2-external) want > is actual testing, with full functionality and non-regression suites and > what-have-you. I definitely see your point. This doesn't replace runtime tests for specific scenarios. > Also, the rationale you provided, testing the tftpy package, is not > appropriate here: tftpy is in Buildrooot, so its runtime test should > also be there, so writing a runtime test in a br2-external does not > make sense in this specific case. This one I can describe. We have a Python package that relies on tftpy. Buildroot doesn't want our own Python package, but probably could benefit from having tftpy. So I submitted a patch with tftpy a month or two ago, and Thomas suggested I write a test, which he just applied. Thanks! But now I'm in limbo. I plan to LTS-hop so there's a couple months where my BR2_EXTERNAL will have tftpy (because I'm on 2023.02.8) and it might be nice to develop / run tests in the meantime. It also would be nice to be to develop tests regardless of whether the patch is accepted upstream. I submitted a bootpc patch a while back, but there's no active maintainer of that project so I wouldn't blame anyone for rejecting it. > Of course, I said I was conflicted about this: I have no strong opinion > against the feature itself; I just don't feel in favour either... Other > maintainers will have to weight in. > > However, there is a case for looking the code, so let's go there. > > First, don't forget that BR2_EXTERNAL is a space-separated list of > directories. Your current patch only considers a single directory will > be used, which is generally not true. Good point. Yes, this will need a revision. > Second, and most importantly, I am not too happy about the need to do > some copying around. It is very often that I hit Ctrl-C to quickly kill > a set of runtime tests, and that would not be caught by the try-finally > clause (the first Ctrl-C would, not the second). So that could leave > quite a lot of directories around; for long-running machines like > build-farm servers, this is going to be a problem... > > Also, tempfile.TemporaryDirectory() uses the same rules as mkdtemp(), > which uses the same rules as mkstemp(), to decide where to create the > temporary directory. On many machine, that resorts to using on of the > TMPDIR, TEMP or TMP environment variables, and it is not unusual for > those to point to .tmp, which in turns is very often a tmpfs, so does > not get a lot of space in there; even if only the infra scripts are > copied, ad those are not so huge, it is still not very nice to leave > around... > > So, the question is: why do we need to copy? > > I guess the reason is that nose2 can only have one starting directory > where it looks for tests, right? Exactly, yes. My first go at it was to just override the test directory. Python dependencies stopped that dead in its tracks. I looked into nose2 sources as well to see if there was a flag or something that could be added. Unfortunately I didn't find any way to do that. > ... > Of particular note, I came up with using symlinks, so that the packages > tests would be eventually discovered by nose2; > > https://gitlab.com/ymorin/buildroot/-/commit/1d9fc876483727c77ec93b4657cd5b4884e2adbb#a66a9b8b0912024dc207cec14b32e038e0c208eb > > Basically, what that would do, is scan the package directory for files > named 'PACKAGE_NAME.py' and create a symlink test_PACKAGE_NAME.py in the > runtime test infra. > > I wonder if that would not be a better solution... I do like the idea of symlinking. I feel like that would break 'git status' if those were done in the buildroot working directory. I'm not opposed to this idea at all. Maybe those symlinks could have a pattern that matches in .gitignore? > > I have also not looked in details at the patch, because the change in > indentation due to the try-finally, is difficult to follow. If we are to > go with a solution similar to what you propose, then it would be nice to > have the patch split in at least three patches: > - one to introduce the try-finally, without any other change > - one to do the copy of the infra unconditionally > - one to copy each of the br2-external trees' test suites Good idea. If the full-copy method is acceptable it would be easier to just always copy. > > But please don't respin too fast, let the others look and chime with > their opinions and reviews... Will do. And forgive me if I don't respond / re-spin in a timely manner. There's a family addition coming any day, so my schedule in January might not allow me to jump into this right away. > > Regards, > Yann E. MORIN. > _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-12-23 17:18 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-12-22 23:22 [Buildroot] [RFC v1 0/1] Add external test support Colin Foster 2023-12-22 23:22 ` [Buildroot] [RFC v1 1/1] support/testing/run-tests: add ability to run tests from external Colin Foster 2023-12-23 13:40 ` Thomas Petazzoni via buildroot 2023-12-23 16:12 ` Colin Foster 2023-12-23 13:54 ` Yann E. MORIN 2023-12-23 17:18 ` Colin Foster
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox