Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add a case which are adding disks with all combianations of cache, bus and format #1212

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

yunmingyang
Copy link
Contributor

No description provided.

@yunmingyang yunmingyang force-pushed the testAddAllTypeDisks branch 2 times, most recently from ac3e05f to ff2e010 Compare September 18, 2023 01:09
Copy link
Contributor

@skobyda skobyda left a comment

Choose a reason for hiding this comment

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

arch and debian-testing are failing. Looks like the disk is not visible after attaching to the VM

@yunmingyang
Copy link
Contributor Author

Not reproduce locally, then add some debug log to see whether it helps

Copy link
Collaborator

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

What's the reason behind this? Was there some related bug?

I would like to think of this test suite as non-exhaustive, rather covering all possible code paths. By covering the grid map of all possible options, we just increase the time that tests run without necessarily increasing test code coverage. Does this make sense?

testAddDiskAdditionalOptions already tests all the options, just not the whole grid.
Testing that options don't break each other is something that could theoretically be part of the libvirt tests. Here we are testing that the UI works as expected, and not all possible libvirt VM configurations.

@yunmingyang
Copy link
Contributor Author

The reason is there is a manual case about this, and if executing this case manually, it will take about 1 ~ 2 hours, even more time. So automating it is the best practice. However, no directly related bug for this automated case, but, I think maybe we could use this case to prevent bugs like https://bugzilla.redhat.com/show_bug.cgi?id=1784297 or https://bugzilla.redhat.com/show_bug.cgi?id=1948366. Although, for now, this case just cover disks in the dir type storage pool, we could abstract it into a function just like _testBasic in the future, to cover more scenarios.

@yunmingyang yunmingyang requested a review from KKoukiou October 16, 2023 03:39
@martinpitt martinpitt marked this pull request as draft November 3, 2023 05:42
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.

3 participants