[1LP][RFR] Add test_compare_provider_templates to test_providers.py #9424
Conversation
prichard77
changed the title
prichard77
changed the title
prichard77
force-pushed
the
compareTemplates
branch
from
edcc923
to
27bdcea
Compare
…_provider_templates to use it
prichard77
force-pushed
the
compareTemplates
branch
from
f29ac93
to
488f3ad
Compare
prichard77
changed the title
|
|
||
|
|
||
| def verify_checked_items_compared(checkedList, view): | ||
| for e in view.comparison_table.headers[1:-1]: |
There was a problem hiding this comment.
Please clean up these magic numbers, indexing, and using single character vars.
What is e? standing for here?
What are you indexing out of the headers? Let's comment in-line and name it more descriptively.
Below, what's the function of the split and zero-index? Again let's descriptively name what this value is.
Why use try/except around a remove, instead of directly asserting that the item from the checked list is in the list of header values?
I think you could use two set objects to verify the list of headers and list of items that had been selected are the same.
Consider having this as a method in the ComparisonView class, instead of just a function within the test module. That way its re-usable across the various entities in MIQ that can be compared, and can be used outside of this test module.
There was a problem hiding this comment.
I'll make the vars more descriptive, add some inline comments. I could put in a function like get_templates_on_page() and put the 'magic' in there. This is all a result of what is contained in .headers. The values returned, except for the fitst and last are the templates in the view. The first and last items are just part of the table. So I used [1:-1] on these. I'll add an inline comment. Then the template-like names have additional characters added to them so I split on space and take the first([0]) item. I'll make this clearer.
I was using try/except because it enabled me to distinguish between missing and extra templates in the view and would stop as soon as a failure was found. But "set" might be a good solution also. I'll look into it.
I agree with putting this functionality into methods for the view classes, and am working on this now in another PR. I am starting with implementing comparison for hosts while investigating what would be needed to extend to templates and datastores. I'd really like to get this test merged and running while I work on the optimization.
The changes are forthcoming, but I wanted to continue the discussion. Thanks.
There was a problem hiding this comment.
I pushed some changes but realize we have more discussion and changes to come.
There was a problem hiding this comment.
I take it you are suggesting creating sets from the 2 lists and taking the symmetric difference. If It's non-zero we have a failure. Only thing is, it takes more steps to determine which lists have the extra/missing items. Or are you suggesting something else?
mshriver
changed the title
| setup_or_skip(request, provider) | ||
|
|
||
|
|
||
| def verify_checked_items_compared(checkedList, view): |
There was a problem hiding this comment.
TODO: move this method into the comparison view.
There was a problem hiding this comment.
Done as requested.
| @pytest.fixture | ||
| def setup_provider_min_templates(request, appliance, provider, min_templates): | ||
| templates_yaml = len(provider.data.get('templates', {})) | ||
| if templates_yaml < min_templates: |
There was a problem hiding this comment.
I think you'll want to skip this check for this test context, as the number of templates in the yaml is almost always going to be lower than the actual number of templates available. We only define in the yaml the templates that we use for provisioning tests, and I think this ends up too restrictive - currently skipping for 4 templates against both versions of rhv and scvmm.
There was a problem hiding this comment.
Updated as requested.
|
I detected some fixture changes in commit 7d29557 The local fixture
Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃 |
…y_checked_items_compared under TemplatesCompareView
prichard77
changed the title
mshriver
changed the title
|
I am not sure what is missing here and why this is not in RFR. I'll keep looking and see what I can determine. |
|
I can't find any reason this is not in RFR. Updating to RFR. |
prichard77
changed the title
Purpose or Intent
PRT Run
{{ pytest: --use-provider complete --long-running cfme/tests/infrastructure/test_providers.py::test_compare_provider_templates}}