Re: [PATCH 09/17] test/py: Add a test image for Ubuntu

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 5017 bytes --]

On Wed, Apr 02, 2025 at 06:55:02AM +1300, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 2 Apr 2025 at 05:49, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Apr 02, 2025 at 04:49:18AM +1300, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Mon, 31 Mar 2025 at 03:45, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Thu, Mar 20, 2025 at 03:41:33AM +0000, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Wed, 19 Mar 2025 at 16:35, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Wed, Mar 19, 2025 at 03:04:28PM +0000, Simon Glass wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Wed, 19 Mar 2025 at 15:52, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Wed, Mar 19, 2025 at 03:38:03PM +0100, Simon Glass wrote:
> > > > > > > >
> > > > > > > > > Add an extlinux image that contains a few Ubuntu entries.
> > > > > > > > >
> > > > > > > > > Increase the number of sandbox-USB-hub ports to permit this.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > >
> > > > > > > > I don't understand what this test adds. In neither the current Fedora
> > > > > > > > test nor in this new test are we actually booting something, we're just
> > > > > > > > taking a sample extlinux.conf and making sure it doesn't fail. Is it
> > > > > > > > that we're testing in a useful fashion now having two labels?
> > > > > > >
> > > > > > > I didn't think so either, which is why I never did this before. But it
> > > > > > > turns out that there were some bugs, too.
> > > > > >
> > > > > > I don't understand you, sorry. You don't think so either to what?
> > > > >
> > > > > I didn't think we needed two extlinux examples on different boot
> > > > > devices, but we do.
> > > >
> > > > I'm not sure it's particularly clear what you're doing here then, or
> > > > why, but I'll find some time to read it all more deeply.
> > >
> > > OK. This test case is how I found the bugs/problems in bootstd that
> > > are fixed in this series.
> >
> > It should be (a) better explained and likely (b) instead of dropping in
> > a seemingly verbatim installed file a specially crafted bit of content.
> > We aren't testing "Ubuntu". We are testing multiple labels.
> >
> > > > > > > > We should probably be clear about what we're doing in the tests and
> > > > > > > > instead of adding seemingly arbitrary distributions add an extlinux test
> > > > > > > > and testcases.
> > > > > > >
> > > > > > > This is not actually a test case. It is simply creating a new image.
> > > > > > > The test cases are in the other patches, so please take a look there.
> > > > > >
> > > > > > Nothing in this series quickly reads as adding tests and fixing problems
> > > > > > with extlinux parsing, it's all bootmeth stuff?
> > > > >
> > > > > It isn't about the actual parsing of the .conf file, although I would
> > > > > like to add tests there as we have none apart from what I have added
> > > > > in my PXE series. It's more about having multiple devices with
> > > > > bootable OSes on them. This series tidies up and fixes this. We need
> > > > > to have an image available on more than one device to spot these
> > > > > problems.
> > > >
> > > > And the existing tests for pxelinux that we have in mainline already,
> > > > don't forget those.
> > >
> > > Yes. But those tests actually don't use bootstd, do they?
> >
> > No, they're testing pxelinux, the thing you said we don't have any tests
> > for.
> >
> > > > > Currently we have two accessible to sandbox, one extlinux and one
> > > > > EFI*. I decided to add a third, using extlinux.
> > > > >
> > > > > Again, this is not a test case, but provides an image for the test
> > > > > cases in this series.
> > > >
> > > > Adding mocked up things for use in sandbox is adding test cases.
> > >
> > > One is a test image for use by tests; the other is a test. Perhaps you
> > > are just saying that there is no point in having one without the
> > > other? Otherwise, I'm not sure what to do with this feedback.
> >
> > Maybe I clarified better above now. You're not testing Ubuntu (or Fedora
> > or Armbian or ...) you're seeing if various pxelinux files parse
> > correctly. Making the tests be clearer about what's being tested is
> > what's missing at least in part.
> 
> You want me to remove the word 'Ubuntu' from the test files? This is
> what I get when I install u-boot-tools so I am trying to use that,
> rather than invent my own thing. This is the same approach I've taken
> with Fedora and Armbian and I think it makes the most sense.

I seem to be unable to explain that if you're constructing tests, I'd
like to see it clear that you're constructing test files. You're not
testing "Ubuntu" or "Fedora" or "Armbian" here. I'm not nak'ing this
patch as at the end of the day, it's adding some test, even if it's also
I believe being done in confusing ways.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]