On Tue, Jan 20, 2026 at 07:15:08PM +0100, Quentin Schulz wrote: > Hi Tom, > > On 1/20/26 6:55 PM, Tom Rini wrote: > > On Tue, Jan 20, 2026 at 06:36:33PM +0100, Quentin Schulz wrote: > > > Hi Tom, > > > > > > On 1/20/26 4:47 PM, Tom Rini wrote: > > > > From: Dmitrii Sharshakov <[email protected]> > > > > > > > > Check if elftools package is available before running DecodeElf(). > > > > > > > > This clarifies the error message and adds the required test for > > > > coverage. > > > > > > > > Signed-off-by: Dmitrii Sharshakov <[email protected]> > > > > Signed-off-by: Andrew Soknacki <[email protected]> > > > > Reviewed-by: Tom Rini <[email protected]> > > > > [trini: Add the test provided by Andrew on IRC, to fix coverage] > > > > Signed-off-by: Tom Rini <[email protected]> > > > > --- > > > > Changes in v3: > > > > - Add the test, provided by Andrew, to address the coverage failure in > > > > CI > > > > --- > > > > tools/binman/elf.py | 2 ++ > > > > tools/binman/elf_test.py | 12 ++++++++++++ > > > > 2 files changed, 14 insertions(+) > > > > > > > > diff --git a/tools/binman/elf.py b/tools/binman/elf.py > > > > index 6ac960e04196..899c84ad36d6 100644 > > > > --- a/tools/binman/elf.py > > > > +++ b/tools/binman/elf.py > > > > @@ -570,6 +570,8 @@ def is_valid(data): > > > > Returns: > > > > bool: True if a valid Elf file, False if not > > > > """ > > > > + if not ELF_TOOLS: > > > > + raise ValueError("Python: No module named 'elftools'") > > > > try: > > > > DecodeElf(data, 0) > > > > return True > > > > diff --git a/tools/binman/elf_test.py b/tools/binman/elf_test.py > > > > index 5b1733928986..3ad0bf4c4b09 100644 > > > > --- a/tools/binman/elf_test.py > > > > +++ b/tools/binman/elf_test.py > > > > @@ -373,6 +373,18 @@ class TestElf(unittest.TestCase): > > > > self.assertEqual(True, elf.is_valid(data)) > > > > self.assertEqual(False, elf.is_valid(data[4:])) > > > > + def test_is_valid_fail(self): > > > > + """Test calling is_valid() without elftools""" > > > > + old_val = elf.ELF_TOOLS > > > > + try: > > > > + elf.ELF_TOOLS = False > > > > + with self.assertRaises(ValueError) as e: > > > > + elf.is_valid(b'') > > > > + self.assertIn("Python: No module named 'elftools'", > > > > + str(e.exception)) > > > > + finally: > > > > + elf.ELF_TOOLS = old_val > > > > + > > > > > > I'm not sure this is a good idea. The issue is that you're modifying the > > > variable from a python module. The check may be run by other tests at the > > > same time in different threads and I think the tests will unnecessarily be > > > skipped or fail? > > > > > > I think we may want something like: > > > > > > @unittest.mock.patch('binman.elf.ELF_TOOLS', False) > > > def test_is_valid_fail(self): > > > ? > > > > It's following the same practice as all of the other tests however. I > > Which is probably incorrect. Also not sure about the os.environ > modification... probably should also be a global or temporary mock.
It's well beyond my scope of expertise at this point, yes.
> > might do a follow-up to try what you suggest, since learning about how
> > to fix this problem yesterday I realized that I can fix I think my "no
> > cover" in commit 66be03b7ee19 ("binman: blob_dtb: improve error message
> > when SPL is not found") correctly now, as I think it's the same kind of
> > failure.
> >
>
> Yeah, you can likely mock self.FdtContents() to return None, None to trigger
> the FileNotFoundError exception with the desired parameters. The only issue
> is whether self.FdtContents is called multiple times within the same
> function (maybe from functions/methods called from that function), then it
> gets harder.
>
> > Personally, I find the exercise as an example of testing for tests sake.
>
> Coverage test is difficult. Sometimes it's unnecessary tests so it makes the
> coverage tool happy we don't regress. How does one decide which tests mean
> something /me shrugs.
Yes, I agree it's a hard task to solve, and sometimes you just need to
put in a test to make the tool happy.
> > We didn't (and can't?) have a test that caught the real problem, which
> > is a lack of elftools giving a hard to understand error message. The
> > patch from Dmitrii fixes that real problem (re-use the test+raise logic
> > other functions do), but python testing requests a test for this new
> > failure.
> >
>
> I haven't looked much into it, but reading elf.py, I didn't like that some
> methods require elftools and some not. I'm wondering if we shouldn't split
> them in two and have the one that relies on elftools not even check if it's
> there. It imports the module, done. If it cannot, it'll complain about
> NameError: name 'ELFFile' is not defined
> NameError: name 'ELFError' is not defined. Did you mean: 'EOFError'?
> and it's up to the caller to handle whether it should be a hard failure (not
> catch it) or not?
> or we could also just raise an Exception to tell the user to install
> pyelftools if we really need a hint at what to do when the exception is
> raised.
That's one of the first things I noticed as well when I saw v1 of this
patch and wanted to improve it. I agree splitting the file and having
one try/raise would be better, and split the requires elftools code from
the doesn't require elftools code.
--
Tom
signature.asc
Description: PGP signature
