Skip to content

Conversation

@glehmann
Copy link
Member

No description provided.

@glehmann glehmann requested a review from a team as a code owner November 27, 2025 20:04
@glehmann glehmann force-pushed the gln/vdi-export-integrity branch 2 times, most recently from 230f06d to 9c9cc72 Compare November 28, 2025 10:08
@glehmann glehmann force-pushed the gln/xva-export-integrity branch from 04dc842 to 40b31bf Compare November 29, 2025 22:11
@glehmann glehmann force-pushed the gln/vdi-export-integrity branch from 9c9cc72 to 92c11bb Compare November 29, 2025 22:11
Copy link

@rzr rzr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't duplicated code be factorized ? or this is not desired in context of pytests ?

https://en.wikipedia.org/wiki/Don%27t_repeat_yourself

@stormi
Copy link
Member

stormi commented Dec 1, 2025

Can't duplicated code be factorized ? or this is not desired in context of pytests ?

https://en.wikipedia.org/wiki/Don%27t_repeat_yourself

We do apply this principle in general, I don't know why not in this specific case, but I agree, we could definitely move the repeated code to a utility function, as we've done for live migration tests in the same storage test packages.

@glehmann
Copy link
Member Author

glehmann commented Dec 8, 2025

Can't duplicated code be factorized ? or this is not desired in context of pytests ?
en.wikipedia.org/wiki/Don%27t_repeat_yourself

We do apply this principle in general, I don't know why not in this specific case, but I agree, we could definitely move the repeated code to a utility function, as we've done for live migration tests in the same storage test packages.

It's just because at first there was a significant number of differences, but most of them are gone. I'll move it to a utility function

@glehmann glehmann force-pushed the gln/xva-export-integrity branch from 40b31bf to b0b5cde Compare December 9, 2025 08:21
@glehmann glehmann force-pushed the gln/vdi-export-integrity branch 2 times, most recently from 5e92156 to 6fe53b8 Compare December 11, 2025 10:31
@glehmann glehmann force-pushed the gln/xva-export-integrity branch from b0b5cde to 5d82813 Compare December 11, 2025 10:31
@glehmann glehmann requested a review from a team December 11, 2025 14:46
@glehmann glehmann force-pushed the gln/xva-export-integrity branch from 5d82813 to 37110a4 Compare December 15, 2025 10:26
@glehmann glehmann force-pushed the gln/vdi-export-integrity branch from 6fe53b8 to 17fc45a Compare December 15, 2025 10:26
@glehmann glehmann force-pushed the gln/xva-export-integrity branch from 37110a4 to f6c3f34 Compare December 15, 2025 11:19
@glehmann glehmann force-pushed the gln/vdi-export-integrity branch from 17fc45a to 9ddd3f9 Compare December 15, 2025 11:19
@glehmann glehmann force-pushed the gln/xva-export-integrity branch from f6c3f34 to ef699b4 Compare December 15, 2025 12:55
@glehmann glehmann force-pushed the gln/vdi-export-integrity branch from 9ddd3f9 to 26a1a1d Compare December 15, 2025 12:55
imported_vm.destroy()
vm.host.ssh(f'rm -f {xva_path}')

ImageFormat = Literal['vhd', 'qcow2']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might have its place in lib/vdi.py?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use it for the type of the parameter of create_vdi too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice indeed, but validating the type from xe requires some extra work that I began in https://github.com/xcp-ng/xcp-ng-tests/pull/317/commits, and that I might re-open

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see #374


def vdi_export_import(vm: VM, sr: SR, image_format: ImageFormat):
vdi = sr.create_vdi(image_format=image_format)
image_path = f'/tmp/{vdi.uuid}.{image_format}'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have seen usage of /tmp elsewhere will it make the test re-entrant ? may safer tmp dir used to avoid potential overlap

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The usage of vdi's uuid should prevent any problem on that side.
Note that this is more for the future though, as the tests are currently mostly sequential.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it is not blocking, just something that popped to my mind while looking the code

I would be tempted to write in a more volatile space, like in a local tmp folder job directory ? this way we are sure those files will not remain if not deleted by test, it can be done in later PR more globally

@glehmann glehmann force-pushed the gln/xva-export-integrity branch from ef699b4 to 8993a77 Compare December 17, 2025 10:18
@glehmann glehmann force-pushed the gln/vdi-export-integrity branch from 26a1a1d to a3c5bba Compare December 17, 2025 10:18
@glehmann glehmann force-pushed the gln/xva-export-integrity branch from 8993a77 to 26128a1 Compare December 17, 2025 10:26
@glehmann glehmann force-pushed the gln/vdi-export-integrity branch from a3c5bba to 6c77a9b Compare December 17, 2025 10:26
Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>
Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>
Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>
Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>
Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>
Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>
Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>
Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>
@glehmann glehmann force-pushed the gln/vdi-export-integrity branch from 6c77a9b to 5d6a4d4 Compare December 19, 2025 17:58
@glehmann glehmann force-pushed the gln/xva-export-integrity branch from 26128a1 to 22c1258 Compare December 19, 2025 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants