fix: MachineType f-string

[Chris-Peterson444]

Fixes MachineType key to be an f-string instead of a regular string

It looks like when things were changed over to f-strings this one line missed the "f". I did some basic searches through the rest of the code to find other instances of this but couldn't find any.

[bdrung]

Thanks for your contribution and catching the failure. Can you add or extend our test suite to cover this failure? Let me know if you need assistance there.

The commit has no description. Can you include the PR description as description for your commit.

nitpick: Can you use "f-string" (instead of "fstring") everywhere (especially in the git commit title)?

[Chris-Peterson444]

The commit has no description. Can you include the PR description as description for your commit.

I took the body of my first comment and added it to the description of the commit. I left the title alone because it seems like the convention is feature (e.g. "fix:", "ci:", etc) commits.

nitpick: Can you use "f-string" (instead of "fstring") everywhere (especially in the git commit title)?

Done.

Thanks for your contribution and catching the failure. Can you add or extend our test suite to cover this failure? Let me know if you need assistance there.

I would appreciate some assistance and/or clarity on this. Do you mean that you like to see a test of attach_hardware and that all of the fields of the report are filled as expected? This I can do. Checking for potentially invalid f-strings would be another thing though.

[bdrung]

Thanks for your contribution and catching the failure. Can you add or extend our test suite to cover this failure? Let me know if you need assistance there.

I would appreciate some assistance and/or clarity on this. Do you mean that you like to see a test of attach_hardware and that all of the fields of the report are filled as expected?

Yes.

Checking for potentially invalid f-strings would be another thing though.

That task would be asked too much and probably not the correct approach to unit testing.

Another nitpick: You could mention the culpit commit 5a50433 in a "Fixes" line. Example: https://github.com/canonical/apport/commit/3c1bf853d8e691766f75f13e8ff625487f8b2f07.patch

[codecov]

Codecov Report

Merging #226 (f303ea4) into main (d20d68b) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #226   +/-   ##
=======================================
  Coverage   82.98%   82.98%           
=======================================
  Files          92       92           
  Lines       18748    18748           
=======================================
  Hits        15558    15558           
  Misses       3190     3190           

Files	Coverage Δ
apport/hookutils.py	62.75% <100.00%> (ø)
tests/unit/test_hookutils.py	100.00% <ø> (ø)

[Chris-Peterson444]

That task would be asked too much and probably not the correct approach to unit testing.

Agreed, but I wanted to sure!

Another nitpick

Done!

I'm not a huge fan of how much patching went into this, but in my opinion testing here should be about making sure any dynamically created keys (like MachineType) are interpolated correctly. That being said I put this into the unit tests and mocked all the other data. We could put this into integration test but this seemed to be the simpler approach.

Lastly I refactored out collecting udevdb information. We should write a separate test to guarantee the anonymization is working, but probably at a later date.

[bdrung]

Wow, that test case has a lot of mocking. The attach_hardware function is doing too much low-level things which should be moved out into separate functions (in future MR).

idea: Setting MachineType could be moved into attach_dmi since it is derived from there. Then attach_dmi could be mocked with real data.

[bdrung]

I'll prepare that test case.

[Chris-Peterson444]

I rebased this branch to include some edits to make the linters happy and cleanup my print statement (whoops).

Wow, that test case has a lot of mocking...Then attach_dmi could be mocked with real data.

Would it be better to move testing of attach_hardware to integration testing then?

[Chris-Peterson444]

Thanks @bdrung. A much simpler PR now, thanks to #227.