| Message ID | 20231222232251.12786-2-colin.foster@in-advantage.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add external test support | expand |
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
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
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
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. >
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__":
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(-)