Re: [Buildroot] [RFC v1 1/1] support/testing/run-tests: add ability to run tests from external

[Colin Foster <colin.foster@in-advantage.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.
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot