Add fw upgrade test #93

nancyc12 · 2023-09-08T15:15:44Z

Description

This is the prototype for firmware upgrade test. It will be temporarily placed in /extras folder until we figure out a proper way to merge it into device agent codes.

This tool relies on DMI chassis type and chassis vendor to determine which firmware update method should be applied on the device. Currently, it only supports fwupd/LVFS update method on selected device types (DMI chassis type: All In One, Desktop) with selected OEM (HP, Dell, and Lenovo). Note that it hasn't been widely tested on all supported devices.

More detailed usages can be found in README.

Output logs

Here're the logs for two devices. Logs have been renamed (default: upgrade_fw.log) to reflect the different actions.

HP Z4 G4 workstation (202102-28691) chassis_type: Desktop
Dell Optiplex AIO Plus 7410 (202209-30589) chassis_type: All In One
upgrade_fw_logs.zip

Test

It passed ruff check * and unit tests as below.

nancy-chen@Ubuntu:~/git/testflinger/extras$ python -m coverage run -m pytest .
================================================================================= test session starts =================================================================================
platform linux -- Python 3.10.12, pytest-7.4.1, pluggy-0.13.0
rootdir: /home/nancy-chen/git/testflinger
configfile: pyproject.toml
plugins: cov-4.1.0
collected 7 items                                                                                                                                                                     

devices/LVFS/tests/test_LVFS.py ...                                                                                                                                             [ 42%]
tests/test_upgrade_fw.py ....                                                                                                                                                   [100%]

================================================================================== 7 passed in 0.04s ==================================================================================

nadzyah

Thanks for your contribution. I hope these comments will help you to improve the code!

extras/devices/LVFS/LVFS.py

extras/devices/LVFS/tests/test_LVFS.py

extras/devices/base.py

extras/upgrade_fw.py

plars

Thanks for working on this! I added a few more comments below.

extras/.gitignore

extras/devices/LVFS/LVFS.py

plars · 2023-09-08T18:08:12Z

extras/devices/LVFS/LVFS.py

+        self.run_cmd("sudo fwupdmgr refresh --force")
+        rc, stdout, stderr = self.run_cmd("sudo fwupdmgr get-devices --json")
+        logger.debug("$fwupdmgr get-devices = \n%s" % stdout)
+        for dev in json.loads(stdout)["Devices"]:


You can simplify this a lot, and also simplify TestLVFSDevice (see the comment from @nadzyah ) significantly by putting everything below here into a func or method, and feeding it the stdout that you get from the command above. Then instead of having to mock command line args, you can just give it test data and confirm that you get back the expected result.

Done. Thanks for the suggestion :)

nadzyah

Thanks for your changes from my previous review. I hope the following once also will help you to improve the code!

extras/devices/base.py

extras/devices/LVFS/LVFS.py

extras/devices/LVFS/tests/fwupd_data.py

extras/devices/LVFS/tests/test_LVFS.py

extras/tests/test_upgrade_fw.py

extras/upgrade_fw.py

plars · 2023-09-12T13:35:16Z

extras/devices/LVFS/LVFS.py

+        logger.info("collect firmware info")
+        self.run_cmd("sudo fwupdmgr refresh --force")
+        rc, stdout, stderr = self.run_cmd("sudo fwupdmgr get-devices --json")
+        logger.debug(f"$fwupdmgr get-devices = \n{stdout}")


sorry, I should have been more specific about the f-strings. Logging is one place where you generally do NOT want to use f-strings, but use the built-in % syntax instead. It's weird, I know. Many times, it doesn't make a big difference, but it's a good habit to get into.

I'm curious about this. Why is it better to use %s instead in logging?

As I understand, the logging library is optimised to use the %s formatting style. I've found these links about it:
https://docs.python.org/3/howto/logging.html#optimization
https://docs.python.org/3/howto/logging-cookbook.html#formatting-styles

extras/devices/LVFS/LVFS.py

plars · 2023-09-12T13:58:48Z

extras/devices/LVFS/tests/test_LVFS.py

+        with patch("devices.LVFS.LVFS.LVFSDevice.run_cmd") as mock_path:
+            mock_path.side_effect = self.mock_run_cmd
+            device = LVFSDevice("", "", "")
+            print(type(fwupd_data.get_device))


Generally, using print() in test cases is not very helpful. I'm guessing you were trying to get some debug info with this? Did you intend to leave it?

Oh I forgot to remove it. Removed.

extras/devices/LVFS/LVFS.py

nadzyah

Thanks for your contribution. +1 from me

plars

+1 from me too, thanks for working on this!

nancyc12 added 2 commits September 8, 2023 22:37

fw test initial commit

6e9046d

update README

c4d363e

nancyc12 requested review from pieqq and plars September 8, 2023 15:16

github-actions bot added the Libraries: Out of sync label Sep 8, 2023

nadzyah requested changes Sep 8, 2023

View reviewed changes

plars requested changes Sep 8, 2023

View reviewed changes

enhance as per feedback

6760e45

nadzyah requested changes Sep 12, 2023

View reviewed changes

plars requested changes Sep 12, 2023

View reviewed changes

nancyc12 added 2 commits September 13, 2023 18:21

add doc-strings for unit test cases and refine constants' names

0c03538

code enhancement

fd8d12c

nadzyah reviewed Sep 13, 2023

View reviewed changes

extras/devices/LVFS/LVFS.py Outdated Show resolved Hide resolved

correct logger % syntax

87f3156

nadzyah approved these changes Sep 13, 2023

View reviewed changes

plars approved these changes Sep 13, 2023

View reviewed changes

plars merged commit 2ed4fa7 into main Sep 13, 2023

plars deleted the add-fw-scripts branch September 13, 2023 20:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fw upgrade test #93

Add fw upgrade test #93

nancyc12 commented Sep 8, 2023

nadzyah left a comment

plars left a comment

plars Sep 8, 2023

nancyc12 Sep 12, 2023

nadzyah left a comment

plars Sep 12, 2023

pieqq Sep 13, 2023

nadzyah Sep 13, 2023

plars Sep 12, 2023

nancyc12 Sep 13, 2023 •

edited

Loading

nadzyah left a comment

plars left a comment

Add fw upgrade test #93

Add fw upgrade test #93

Conversation

nancyc12 commented Sep 8, 2023

Description

Output logs

Test

nadzyah left a comment

Choose a reason for hiding this comment

plars left a comment

Choose a reason for hiding this comment

plars Sep 8, 2023

Choose a reason for hiding this comment

nancyc12 Sep 12, 2023

Choose a reason for hiding this comment

nadzyah left a comment

Choose a reason for hiding this comment

plars Sep 12, 2023

Choose a reason for hiding this comment

pieqq Sep 13, 2023

Choose a reason for hiding this comment

nadzyah Sep 13, 2023

Choose a reason for hiding this comment

plars Sep 12, 2023

Choose a reason for hiding this comment

nancyc12 Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

nadzyah left a comment

Choose a reason for hiding this comment

plars left a comment

Choose a reason for hiding this comment

nancyc12 Sep 13, 2023 •

edited

Loading