canonical / multipass

Multipass orchestrates virtual Ubuntu instances
https://multipass.run
GNU General Public License v3.0
7.63k stars 635 forks source link

Added read cloud-init-config.iso file function #3382

Closed georgeliao closed 6 months ago

georgeliao commented 7 months ago

Added CloudInitIso::read_from function which is the missing piece of the CloudInitIso class.

Please start with cloud_Init_Iso_read_me.md file to get a grasp of the preliminaries.

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (6aed68d) 88.56% compared to head (7a3ac41) 88.62%. Report is 8 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3382 +/- ## ========================================== + Coverage 88.56% 88.62% +0.05% ========================================== Files 251 252 +1 Lines 13859 13949 +90 ========================================== + Hits 12274 12362 +88 - Misses 1585 1587 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

georgeliao commented 6 months ago

@townsend2010 There are two parts of throwing statements.

First part are the ones in the anonymous namespace, they are "private functions" in a way that only can be tested via the public method read_from. It is hard to reach those branches in the scenario. I think I can move them out to be utility functions and test them individually.

Second part are the ones in the read_from function. Those branches requires a malformed cloud-init-config.iso file. For now, write_to function is only function to produce cloud-init-config.iso file and it only makes the healthy-formed ones. We can also write also test utility functions to produce malformed cloud-init-config.iso file and get these branches covered.

townsend2010 commented 6 months ago

Hey @georgeliao,

I don't think there is a need to move those functions out of the anonymous namespace. I think instead you should singletons from FileOps within the read_from() function and then you can inject mocked streams etc that can then be manipulated to have the errors, etc. you'd like for them to do. You see examples of this behavior in the tests using MockFileOps and FileOps.

georgeliao commented 6 months ago

@townsend2010 Indeed, I have overlooked that. That can seep into all the file operation related branches.

georgeliao commented 6 months ago

@townsend2010 , the extra coverage is added based on your advice, the patch coverage now is 100%.

townsend2010 commented 6 months ago

Hey @georgeliao! Thanks for adding the additional unit tests. Could you please rebase this on main in order to get a better coverage report? Thanks!

townsend2010 commented 6 months ago

I've asked @andrei-toterman to review this. Thanks!