diff mbox series

[1/1] test: fix pylint warning for capsule tests

Message ID 20230414083341.288058-1-heinrich.schuchardt@canonical.com
State Accepted, archived
Commit b54477ff8b4f5de286e6ee0d5fd656a4a6eef295
Delegated to: Heinrich Schuchardt
Headers show
Series [1/1] test: fix pylint warning for capsule tests | expand

Commit Message

Heinrich Schuchardt April 14, 2023, 8:33 a.m. UTC
Fix pylint warnings like:

* Class inherits from object
* Missing module description
* Missing class description
* First line of comment blank
* Superfluous imports

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 test/py/tests/test_efi_capsule/conftest.py    | 27 ++++--------
 .../test_capsule_firmware_fit.py              | 35 ++++++++--------
 .../test_capsule_firmware_signed_fit.py       | 41 ++++++++++---------
 .../test_capsule_firmware_signed_raw.py       | 38 ++++++++---------
 4 files changed, 65 insertions(+), 76 deletions(-)

Comments

Simon Glass April 19, 2023, 1:45 a.m. UTC | #1
Hi Heinrich,

On Fri, 14 Apr 2023 at 02:34, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Fix pylint warnings like:
>
> * Class inherits from object
> * Missing module description
> * Missing class description
> * First line of comment blank
> * Superfluous imports
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  test/py/tests/test_efi_capsule/conftest.py    | 27 ++++--------
>  .../test_capsule_firmware_fit.py              | 35 ++++++++--------
>  .../test_capsule_firmware_signed_fit.py       | 41 ++++++++++---------
>  .../test_capsule_firmware_signed_raw.py       | 38 ++++++++---------
>  4 files changed, 65 insertions(+), 76 deletions(-)
>
> diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py
> index 4879f2b5c2..0e5137de60 100644
> --- a/test/py/tests/test_efi_capsule/conftest.py
> +++ b/test/py/tests/test_efi_capsule/conftest.py
> @@ -2,30 +2,21 @@
>  # Copyright (c) 2020, Linaro Limited
>  # Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
>
> -import os
> -import os.path
> -import re
> -from subprocess import call, check_call, check_output, CalledProcessError
> -import pytest
> -from capsule_defs import *
> +"""Fixture for UEFI capsule test
> +"""
>
> -#
> -# Fixture for UEFI capsule test
> -#
> +from subprocess import call, check_call, CalledProcessError
> +import pytest
> +from capsule_defs import CAPSULE_DATA_DIR, CAPSULE_INSTALL_DIR, EFITOOLS_PATH
>
>  @pytest.fixture(scope='session')
>  def efi_capsule_data(request, u_boot_config):
> -    """Set up a file system to be used in UEFI capsule and
> -       authentication test.
> -
> -    Args:
> -        request: Pytest request object.
> -        u_boot_config: U-boot configuration.
> +    """Set up a file system to be used in UEFI capsule and authentication test
> +    and return a ath to disk image to be used for testing

Thanks for cleaning this up. I suppose with all the rounds of review
we got tired of worrying about the style. Also this probably predates
the pylint check.

Can we please follow the style in the rest of the code? This should
have a heading line, with further notes after a blank line.

>
> -    Return:
> -        A path to disk image to be used for testing
> +    request -- Pytest request object.
> +    u_boot_config -- U-boot configuration.

Again the style is:

Return:
   request (pytest.Request): Request to be processed
   u_boot_config (type): U-Boot configuration

(so avoid periods at the end and add the type in brackets)

The u_boot_config thing is pretty annoying since it creates a
strangely named class - see confest.py where is has:

    ubconfig = ArbitraryAttributeContainer()

Really that should be a class with a sensible name and documented
properties. The internals of this are a little too arcane for my
liking and discoverability is not great.

[..]

Also, this should be added to the checker - try 'make pylint' to see that.

Regards,
Simon
Heinrich Schuchardt April 19, 2023, 6:30 a.m. UTC | #2
On 4/19/23 03:45, Simon Glass wrote:
> Hi Heinrich,
> 
> On Fri, 14 Apr 2023 at 02:34, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> Fix pylint warnings like:
>>
>> * Class inherits from object
>> * Missing module description
>> * Missing class description
>> * First line of comment blank
>> * Superfluous imports
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>>   test/py/tests/test_efi_capsule/conftest.py    | 27 ++++--------
>>   .../test_capsule_firmware_fit.py              | 35 ++++++++--------
>>   .../test_capsule_firmware_signed_fit.py       | 41 ++++++++++---------
>>   .../test_capsule_firmware_signed_raw.py       | 38 ++++++++---------
>>   4 files changed, 65 insertions(+), 76 deletions(-)
>>
>> diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py
>> index 4879f2b5c2..0e5137de60 100644
>> --- a/test/py/tests/test_efi_capsule/conftest.py
>> +++ b/test/py/tests/test_efi_capsule/conftest.py
>> @@ -2,30 +2,21 @@
>>   # Copyright (c) 2020, Linaro Limited
>>   # Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>
>> -import os
>> -import os.path
>> -import re
>> -from subprocess import call, check_call, check_output, CalledProcessError
>> -import pytest
>> -from capsule_defs import *
>> +"""Fixture for UEFI capsule test
>> +"""
>>
>> -#
>> -# Fixture for UEFI capsule test
>> -#
>> +from subprocess import call, check_call, CalledProcessError
>> +import pytest
>> +from capsule_defs import CAPSULE_DATA_DIR, CAPSULE_INSTALL_DIR, EFITOOLS_PATH
>>
>>   @pytest.fixture(scope='session')
>>   def efi_capsule_data(request, u_boot_config):
>> -    """Set up a file system to be used in UEFI capsule and
>> -       authentication test.
>> -
>> -    Args:
>> -        request: Pytest request object.
>> -        u_boot_config: U-boot configuration.
>> +    """Set up a file system to be used in UEFI capsule and authentication test
>> +    and return a ath to disk image to be used for testing
> 
> Thanks for cleaning this up. I suppose with all the rounds of review
> we got tired of worrying about the style. Also this probably predates
> the pylint check.
> 
> Can we please follow the style in the rest of the code? This should
> have a heading line, with further notes after a blank line.
> 
>>
>> -    Return:
>> -        A path to disk image to be used for testing
>> +    request -- Pytest request object.
>> +    u_boot_config -- U-boot configuration.
> 
> Again the style is:
> 
> Return:
>     request (pytest.Request): Request to be processed
>     u_boot_config (type): U-Boot configuration

This seems to be some Google internal code style. We should stick to 
PEP257 (https://peps.python.org/pep-0257/).

Best regards

Heinrich

> 
> (so avoid periods at the end and add the type in brackets)
> 
> The u_boot_config thing is pretty annoying since it creates a
> strangely named class - see confest.py where is has:
> 
>      ubconfig = ArbitraryAttributeContainer()
> 
> Really that should be a class with a sensible name and documented
> properties. The internals of this are a little too arcane for my
> liking and discoverability is not great.
> 
> [..]
> 
> Also, this should be added to the checker - try 'make pylint' to see that.
> 
> Regards,
> Simon
Simon Glass April 19, 2023, 10:40 p.m. UTC | #3
Hi Heinrich,

On Wed, 19 Apr 2023 at 18:30, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 4/19/23 03:45, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Fri, 14 Apr 2023 at 02:34, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >> Fix pylint warnings like:
> >>
> >> * Class inherits from object
> >> * Missing module description
> >> * Missing class description
> >> * First line of comment blank
> >> * Superfluous imports
> >>
> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >> ---
> >>   test/py/tests/test_efi_capsule/conftest.py    | 27 ++++--------
> >>   .../test_capsule_firmware_fit.py              | 35 ++++++++--------
> >>   .../test_capsule_firmware_signed_fit.py       | 41 ++++++++++---------
> >>   .../test_capsule_firmware_signed_raw.py       | 38 ++++++++---------
> >>   4 files changed, 65 insertions(+), 76 deletions(-)
> >>
> >> diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py
> >> index 4879f2b5c2..0e5137de60 100644
> >> --- a/test/py/tests/test_efi_capsule/conftest.py
> >> +++ b/test/py/tests/test_efi_capsule/conftest.py
> >> @@ -2,30 +2,21 @@
> >>   # Copyright (c) 2020, Linaro Limited
> >>   # Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>
> >> -import os
> >> -import os.path
> >> -import re
> >> -from subprocess import call, check_call, check_output, CalledProcessError
> >> -import pytest
> >> -from capsule_defs import *
> >> +"""Fixture for UEFI capsule test
> >> +"""
> >>
> >> -#
> >> -# Fixture for UEFI capsule test
> >> -#
> >> +from subprocess import call, check_call, CalledProcessError
> >> +import pytest
> >> +from capsule_defs import CAPSULE_DATA_DIR, CAPSULE_INSTALL_DIR, EFITOOLS_PATH
> >>
> >>   @pytest.fixture(scope='session')
> >>   def efi_capsule_data(request, u_boot_config):
> >> -    """Set up a file system to be used in UEFI capsule and
> >> -       authentication test.
> >> -
> >> -    Args:
> >> -        request: Pytest request object.
> >> -        u_boot_config: U-boot configuration.
> >> +    """Set up a file system to be used in UEFI capsule and authentication test
> >> +    and return a ath to disk image to be used for testing
> >
> > Thanks for cleaning this up. I suppose with all the rounds of review
> > we got tired of worrying about the style. Also this probably predates
> > the pylint check.
> >
> > Can we please follow the style in the rest of the code? This should
> > have a heading line, with further notes after a blank line.
> >
> >>
> >> -    Return:
> >> -        A path to disk image to be used for testing
> >> +    request -- Pytest request object.
> >> +    u_boot_config -- U-boot configuration.
> >
> > Again the style is:
> >
> > Return:
> >     request (pytest.Request): Request to be processed
> >     u_boot_config (type): U-Boot configuration
>
> This seems to be some Google internal code style. We should stick to
> PEP257 (https://peps.python.org/pep-0257/).

That doesn't really tell you much though. It is just the idea of a
docstring from 20 years ago. We need a convention for what is in the
strings, not just the strings.

The one we currently use is at least based on [1] or something like
it. I would not describe it as internal to Google. What makes you
think that? It is widely used.

For me at least, pylint asks for type information:

tools/binman/control.py:146:0: W9016: "modules, test_missing" missing
in parameter type documentation (missing-type-doc)

You should be able to try that and see what you get.

Regards,
Simon

[1] https://sphinxcontrib-napoleon.readthedocs.io/en/latest/index.html
Heinrich Schuchardt April 20, 2023, 5:56 a.m. UTC | #4
On 4/20/23 00:40, Simon Glass wrote:
> Hi Heinrich,
> 
> On Wed, 19 Apr 2023 at 18:30, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>>
>>
>> On 4/19/23 03:45, Simon Glass wrote:
>>> Hi Heinrich,
>>>
>>> On Fri, 14 Apr 2023 at 02:34, Heinrich Schuchardt
>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>
>>>> Fix pylint warnings like:
>>>>
>>>> * Class inherits from object
>>>> * Missing module description
>>>> * Missing class description
>>>> * First line of comment blank
>>>> * Superfluous imports
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>> ---
>>>>    test/py/tests/test_efi_capsule/conftest.py    | 27 ++++--------
>>>>    .../test_capsule_firmware_fit.py              | 35 ++++++++--------
>>>>    .../test_capsule_firmware_signed_fit.py       | 41 ++++++++++---------
>>>>    .../test_capsule_firmware_signed_raw.py       | 38 ++++++++---------
>>>>    4 files changed, 65 insertions(+), 76 deletions(-)
>>>>
>>>> diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py
>>>> index 4879f2b5c2..0e5137de60 100644
>>>> --- a/test/py/tests/test_efi_capsule/conftest.py
>>>> +++ b/test/py/tests/test_efi_capsule/conftest.py
>>>> @@ -2,30 +2,21 @@
>>>>    # Copyright (c) 2020, Linaro Limited
>>>>    # Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>>
>>>> -import os
>>>> -import os.path
>>>> -import re
>>>> -from subprocess import call, check_call, check_output, CalledProcessError
>>>> -import pytest
>>>> -from capsule_defs import *
>>>> +"""Fixture for UEFI capsule test
>>>> +"""
>>>>
>>>> -#
>>>> -# Fixture for UEFI capsule test
>>>> -#
>>>> +from subprocess import call, check_call, CalledProcessError
>>>> +import pytest
>>>> +from capsule_defs import CAPSULE_DATA_DIR, CAPSULE_INSTALL_DIR, EFITOOLS_PATH
>>>>
>>>>    @pytest.fixture(scope='session')
>>>>    def efi_capsule_data(request, u_boot_config):
>>>> -    """Set up a file system to be used in UEFI capsule and
>>>> -       authentication test.
>>>> -
>>>> -    Args:
>>>> -        request: Pytest request object.
>>>> -        u_boot_config: U-boot configuration.
>>>> +    """Set up a file system to be used in UEFI capsule and authentication test
>>>> +    and return a ath to disk image to be used for testing
>>>
>>> Thanks for cleaning this up. I suppose with all the rounds of review
>>> we got tired of worrying about the style. Also this probably predates
>>> the pylint check.
>>>
>>> Can we please follow the style in the rest of the code? This should
>>> have a heading line, with further notes after a blank line.
>>>
>>>>
>>>> -    Return:
>>>> -        A path to disk image to be used for testing
>>>> +    request -- Pytest request object.
>>>> +    u_boot_config -- U-boot configuration.
>>>
>>> Again the style is:
>>>
>>> Return:
>>>      request (pytest.Request): Request to be processed
>>>      u_boot_config (type): U-Boot configuration
>>
>> This seems to be some Google internal code style. We should stick to
>> PEP257 (https://peps.python.org/pep-0257/).
> 
> That doesn't really tell you much though. It is just the idea of a
> docstring from 20 years ago. We need a convention for what is in the
> strings, not just the strings.
> 
> The one we currently use is at least based on [1] or something like
> it. I would not describe it as internal to Google. What makes you
> think that? It is widely used.
> 
> For me at least, pylint asks for type information:
> 
> tools/binman/control.py:146:0: W9016: "modules, test_missing" missing
> in parameter type documentation (missing-type-doc)

You should add type hints (PEP484) and use snake case (PEP8):

-def WriteEntryDocs(modules, test_missing=None):
+def write_entry_docs(modules: List[str], test_missing: str=None):

-def ListEntries(image_fname, entry_paths):
+def list_entries(image_fname: str, entry_paths: List[str]):

Best regards

Heinrich

> 
> You should be able to try that and see what you get.
> 
> Regards,
> Simon
> 
> [1] https://sphinxcontrib-napoleon.readthedocs.io/en/latest/index.html
Simon Glass April 20, 2023, 4:30 p.m. UTC | #5
Hi Heinrich,

On Thu, 20 Apr 2023 at 17:57, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 4/20/23 00:40, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Wed, 19 Apr 2023 at 18:30, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >>
> >>
> >> On 4/19/23 03:45, Simon Glass wrote:
> >>> Hi Heinrich,
> >>>
> >>> On Fri, 14 Apr 2023 at 02:34, Heinrich Schuchardt
> >>> <heinrich.schuchardt@canonical.com> wrote:
> >>>>
> >>>> Fix pylint warnings like:
> >>>>
> >>>> * Class inherits from object
> >>>> * Missing module description
> >>>> * Missing class description
> >>>> * First line of comment blank
> >>>> * Superfluous imports
> >>>>
> >>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >>>> ---
> >>>>    test/py/tests/test_efi_capsule/conftest.py    | 27 ++++--------
> >>>>    .../test_capsule_firmware_fit.py              | 35 ++++++++--------
> >>>>    .../test_capsule_firmware_signed_fit.py       | 41 ++++++++++---------
> >>>>    .../test_capsule_firmware_signed_raw.py       | 38 ++++++++---------
> >>>>    4 files changed, 65 insertions(+), 76 deletions(-)
> >>>>
> >>>> diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py
> >>>> index 4879f2b5c2..0e5137de60 100644
> >>>> --- a/test/py/tests/test_efi_capsule/conftest.py
> >>>> +++ b/test/py/tests/test_efi_capsule/conftest.py
> >>>> @@ -2,30 +2,21 @@
> >>>>    # Copyright (c) 2020, Linaro Limited
> >>>>    # Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>>
> >>>> -import os
> >>>> -import os.path
> >>>> -import re
> >>>> -from subprocess import call, check_call, check_output, CalledProcessError
> >>>> -import pytest
> >>>> -from capsule_defs import *
> >>>> +"""Fixture for UEFI capsule test
> >>>> +"""
> >>>>
> >>>> -#
> >>>> -# Fixture for UEFI capsule test
> >>>> -#
> >>>> +from subprocess import call, check_call, CalledProcessError
> >>>> +import pytest
> >>>> +from capsule_defs import CAPSULE_DATA_DIR, CAPSULE_INSTALL_DIR, EFITOOLS_PATH
> >>>>
> >>>>    @pytest.fixture(scope='session')
> >>>>    def efi_capsule_data(request, u_boot_config):
> >>>> -    """Set up a file system to be used in UEFI capsule and
> >>>> -       authentication test.
> >>>> -
> >>>> -    Args:
> >>>> -        request: Pytest request object.
> >>>> -        u_boot_config: U-boot configuration.
> >>>> +    """Set up a file system to be used in UEFI capsule and authentication test
> >>>> +    and return a ath to disk image to be used for testing
> >>>
> >>> Thanks for cleaning this up. I suppose with all the rounds of review
> >>> we got tired of worrying about the style. Also this probably predates
> >>> the pylint check.
> >>>
> >>> Can we please follow the style in the rest of the code? This should
> >>> have a heading line, with further notes after a blank line.
> >>>
> >>>>
> >>>> -    Return:
> >>>> -        A path to disk image to be used for testing
> >>>> +    request -- Pytest request object.
> >>>> +    u_boot_config -- U-boot configuration.
> >>>
> >>> Again the style is:
> >>>
> >>> Return:
> >>>      request (pytest.Request): Request to be processed
> >>>      u_boot_config (type): U-Boot configuration
> >>
> >> This seems to be some Google internal code style. We should stick to
> >> PEP257 (https://peps.python.org/pep-0257/).
> >
> > That doesn't really tell you much though. It is just the idea of a
> > docstring from 20 years ago. We need a convention for what is in the
> > strings, not just the strings.
> >
> > The one we currently use is at least based on [1] or something like
> > it. I would not describe it as internal to Google. What makes you
> > think that? It is widely used.
> >
> > For me at least, pylint asks for type information:
> >
> > tools/binman/control.py:146:0: W9016: "modules, test_missing" missing
> > in parameter type documentation (missing-type-doc)
>
> You should add type hints (PEP484) and use snake case (PEP8):
>
> -def WriteEntryDocs(modules, test_missing=None):
> +def write_entry_docs(modules: List[str], test_missing: str=None):

PEP8 yes.

>
> -def ListEntries(image_fname, entry_paths):
> +def list_entries(image_fname: str, entry_paths: List[str]):

PEP484 OMG it is so horrible, and you have just shown some simple
cases. See tbot for a code base that uses this and it is not easy.

Regards,
Simon
diff mbox series

Patch

diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py
index 4879f2b5c2..0e5137de60 100644
--- a/test/py/tests/test_efi_capsule/conftest.py
+++ b/test/py/tests/test_efi_capsule/conftest.py
@@ -2,30 +2,21 @@ 
 # Copyright (c) 2020, Linaro Limited
 # Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
 
-import os
-import os.path
-import re
-from subprocess import call, check_call, check_output, CalledProcessError
-import pytest
-from capsule_defs import *
+"""Fixture for UEFI capsule test
+"""
 
-#
-# Fixture for UEFI capsule test
-#
+from subprocess import call, check_call, CalledProcessError
+import pytest
+from capsule_defs import CAPSULE_DATA_DIR, CAPSULE_INSTALL_DIR, EFITOOLS_PATH
 
 @pytest.fixture(scope='session')
 def efi_capsule_data(request, u_boot_config):
-    """Set up a file system to be used in UEFI capsule and
-       authentication test.
-
-    Args:
-        request: Pytest request object.
-        u_boot_config: U-boot configuration.
+    """Set up a file system to be used in UEFI capsule and authentication test
+    and return a ath to disk image to be used for testing
 
-    Return:
-        A path to disk image to be used for testing
+    request -- Pytest request object.
+    u_boot_config -- U-boot configuration.
     """
-    global CAPSULE_DATA_DIR, CAPSULE_INSTALL_DIR
 
     mnt_point = u_boot_config.persistent_data_dir + '/test_efi_capsule'
     data_dir = mnt_point + CAPSULE_DATA_DIR
diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware_fit.py b/test/py/tests/test_efi_capsule/test_capsule_firmware_fit.py
index d28b53a1a1..9ee152818d 100644
--- a/test/py/tests/test_efi_capsule/test_capsule_firmware_fit.py
+++ b/test/py/tests/test_efi_capsule/test_capsule_firmware_fit.py
@@ -1,16 +1,13 @@ 
 # SPDX-License-Identifier:      GPL-2.0+
 # Copyright (c) 2020, Linaro Limited
 # Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
-#
-# U-Boot UEFI: Firmware Update Test
 
-"""
+"""U-Boot UEFI: Firmware Update Test
 This test verifies capsule-on-disk firmware update for FIT images
 """
 
-from subprocess import check_call, check_output, CalledProcessError
 import pytest
-from capsule_defs import *
+from capsule_defs import CAPSULE_DATA_DIR, CAPSULE_INSTALL_DIR
 
 
 @pytest.mark.boardspec('sandbox_flattree')
@@ -24,15 +21,18 @@  from capsule_defs import *
 @pytest.mark.buildconfigspec('cmd_nvedit_efi')
 @pytest.mark.buildconfigspec('cmd_sf')
 @pytest.mark.slow
-class TestEfiCapsuleFirmwareFit(object):
+class TestEfiCapsuleFirmwareFit():
+    """Test capsule-on-disk firmware update for FIT images
+    """
+
     def test_efi_capsule_fw1(
             self, u_boot_config, u_boot_console, efi_capsule_data):
-        """
-        Test Case 1 - Update U-Boot and U-Boot environment on SPI Flash
-                      but with an incorrect GUID value in the capsule
-                      No update should happen
-                      0x100000-0x150000: U-Boot binary (but dummy)
-                      0x150000-0x200000: U-Boot environment (but dummy)
+        """Test Case 1
+        Update U-Boot and U-Boot environment on SPI Flash
+        but with an incorrect GUID value in the capsule
+        No update should happen
+        0x100000-0x150000: U-Boot binary (but dummy)
+        0x150000-0x200000: U-Boot environment (but dummy)
         """
         # other tests might have run and the
         # system might not be in a clean state.
@@ -74,8 +74,6 @@  class TestEfiCapsuleFirmwareFit(object):
 
         capsule_early = u_boot_config.buildconfig.get(
             'config_efi_capsule_on_disk_early')
-        capsule_auth = u_boot_config.buildconfig.get(
-            'config_efi_capsule_authenticate')
 
         # reboot
         u_boot_console.restart_uboot(expect_reset = capsule_early)
@@ -107,11 +105,12 @@  class TestEfiCapsuleFirmwareFit(object):
 
     def test_efi_capsule_fw2(
             self, u_boot_config, u_boot_console, efi_capsule_data):
+        """Test Case 2
+        Update U-Boot and U-Boot environment on SPI Flash
+        0x100000-0x150000: U-Boot binary (but dummy)
+        0x150000-0x200000: U-Boot environment (but dummy)
         """
-        Test Case 2 - Update U-Boot and U-Boot environment on SPI Flash
-                      0x100000-0x150000: U-Boot binary (but dummy)
-                      0x150000-0x200000: U-Boot environment (but dummy)
-        """
+
         disk_img = efi_capsule_data
         with u_boot_console.log.section('Test Case 2-a, before reboot'):
             output = u_boot_console.run_command_list([
diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_fit.py b/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_fit.py
index 8c2d616fd0..ba8429e83c 100644
--- a/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_fit.py
+++ b/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_fit.py
@@ -3,10 +3,8 @@ 
 # Copyright (c) 2022, Arm Limited
 # Author: AKASHI Takahiro <takahiro.akashi@linaro.org>,
 #         adapted to FIT images by Vincent Stehlé <vincent.stehle@arm.com>
-#
-# U-Boot UEFI: Firmware Update (Signed capsule with FIT images) Test
 
-"""
+"""U-Boot UEFI: Firmware Update (Signed capsule with FIT images) Test
 This test verifies capsule-on-disk firmware update
 with signed capsule files containing FIT images
 """
@@ -25,15 +23,18 @@  from capsule_defs import CAPSULE_DATA_DIR, CAPSULE_INSTALL_DIR
 @pytest.mark.buildconfigspec('cmd_nvedit_efi')
 @pytest.mark.buildconfigspec('cmd_sf')
 @pytest.mark.slow
-class TestEfiCapsuleFirmwareSignedFit(object):
+class TestEfiCapsuleFirmwareSignedFit():
+    """Capsule-on-disk firmware update test
+    """
+
     def test_efi_capsule_auth1(
             self, u_boot_config, u_boot_console, efi_capsule_data):
-        """
-        Test Case 1 - Update U-Boot on SPI Flash, FIT image format
-                      0x100000-0x150000: U-Boot binary (but dummy)
+        """Test Case 1
+        Update U-Boot on SPI Flash, FIT image format
+        x150000: U-Boot binary (but dummy)
 
-                      If the capsule is properly signed, the authentication
-                      should pass and the firmware be updated.
+        If the capsule is properly signed, the authentication
+        should pass and the firmware be updated.
         """
         disk_img = efi_capsule_data
         with u_boot_console.log.section('Test Case 1-a, before reboot'):
@@ -103,13 +104,13 @@  class TestEfiCapsuleFirmwareSignedFit(object):
 
     def test_efi_capsule_auth2(
             self, u_boot_config, u_boot_console, efi_capsule_data):
-        """
-        Test Case 2 - Update U-Boot on SPI Flash, FIT image format
-                      0x100000-0x150000: U-Boot binary (but dummy)
+        """Test Case 2
+        Update U-Boot on SPI Flash, FIT image format
+        0x100000-0x150000: U-Boot binary (but dummy)
 
-                      If the capsule is signed but with an invalid key,
-                      the authentication should fail and the firmware
-                      not be updated.
+        If the capsule is signed but with an invalid key,
+        the authentication should fail and the firmware
+        not be updated.
         """
         disk_img = efi_capsule_data
         with u_boot_console.log.section('Test Case 2-a, before reboot'):
@@ -182,12 +183,12 @@  class TestEfiCapsuleFirmwareSignedFit(object):
 
     def test_efi_capsule_auth3(
             self, u_boot_config, u_boot_console, efi_capsule_data):
-        """
-        Test Case 3 - Update U-Boot on SPI Flash, FIT image format
-                      0x100000-0x150000: U-Boot binary (but dummy)
+        """Test Case 3
+        Update U-Boot on SPI Flash, FIT image format
+        0x100000-0x150000: U-Boot binary (but dummy)
 
-                      If the capsule is not signed, the authentication
-                      should fail and the firmware not be updated.
+        If the capsule is not signed, the authentication
+        should fail and the firmware not be updated.
         """
         disk_img = efi_capsule_data
         with u_boot_console.log.section('Test Case 3-a, before reboot'):
diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_raw.py b/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_raw.py
index 2bbaa9cc55..710d9925a3 100644
--- a/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_raw.py
+++ b/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_raw.py
@@ -1,10 +1,8 @@ 
 # SPDX-License-Identifier:      GPL-2.0+
 # Copyright (c) 2021, Linaro Limited
 # Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
-#
-# U-Boot UEFI: Firmware Update (Signed capsule with raw images) Test
 
-"""
+"""U-Boot UEFI: Firmware Update (Signed capsule with raw images) Test
 This test verifies capsule-on-disk firmware update
 with signed capsule files containing raw images
 """
@@ -23,15 +21,17 @@  from capsule_defs import CAPSULE_DATA_DIR, CAPSULE_INSTALL_DIR
 @pytest.mark.buildconfigspec('cmd_nvedit_efi')
 @pytest.mark.buildconfigspec('cmd_sf')
 @pytest.mark.slow
-class TestEfiCapsuleFirmwareSignedRaw(object):
+class TestEfiCapsuleFirmwareSignedRaw():
+    """Firmware Update (Signed capsule with raw images) Test
+    """
+
     def test_efi_capsule_auth1(
             self, u_boot_config, u_boot_console, efi_capsule_data):
-        """
-        Test Case 1 - Update U-Boot on SPI Flash, raw image format
-                      0x100000-0x150000: U-Boot binary (but dummy)
+        """Test Case 1 - Update U-Boot on SPI Flash, raw image format
+        0x100000-0x150000: U-Boot binary (but dummy)
 
-                      If the capsule is properly signed, the authentication
-                      should pass and the firmware be updated.
+        If the capsule is properly signed, the authentication
+        should pass and the firmware be updated.
         """
         disk_img = efi_capsule_data
         with u_boot_console.log.section('Test Case 1-a, before reboot'):
@@ -100,13 +100,12 @@  class TestEfiCapsuleFirmwareSignedRaw(object):
 
     def test_efi_capsule_auth2(
             self, u_boot_config, u_boot_console, efi_capsule_data):
-        """
-        Test Case 2 - Update U-Boot on SPI Flash, raw image format
-                      0x100000-0x150000: U-Boot binary (but dummy)
+        """Test Case 2 - Update U-Boot on SPI Flash, raw image format
+        0x100000-0x150000: U-Boot binary (but dummy)
 
-                      If the capsule is signed but with an invalid key,
-                      the authentication should fail and the firmware
-                      not be updated.
+        If the capsule is signed but with an invalid key,
+        the authentication should fail and the firmware
+        not be updated.
         """
         disk_img = efi_capsule_data
         with u_boot_console.log.section('Test Case 2-a, before reboot'):
@@ -179,12 +178,11 @@  class TestEfiCapsuleFirmwareSignedRaw(object):
 
     def test_efi_capsule_auth3(
             self, u_boot_config, u_boot_console, efi_capsule_data):
-        """
-        Test Case 3 - Update U-Boot on SPI Flash, raw image format
-                      0x100000-0x150000: U-Boot binary (but dummy)
+        """Test Case 3 - Update U-Boot on SPI Flash, raw image format
+        0x100000-0x150000: U-Boot binary (but dummy)
 
-                      If the capsule is not signed, the authentication
-                      should fail and the firmware not be updated.
+        If the capsule is not signed, the authentication
+        should fail and the firmware not be updated.
         """
         disk_img = efi_capsule_data
         with u_boot_console.log.section('Test Case 3-a, before reboot'):