[1LP][RFR] Automating retire bundle with multiple VM test

[niyazRedhat]

{{pytest: cfme/tests/services/test_service_catalogs_bundles.py::test_retire_service_and_bundle_vms --long-running -svvvv }}

[niyazRedhat]

It passing locally every time.

[tpapaioa]

These can be changed to f-strings.

[tpapaioa]

Optional: Is there a need for the index i here? Something like the following would maybe be simpler:

for _ in range(2):
[...]
    cat_list.append(catalog_item)
    request.addfinalizer(catalog_item.delete_if_exists)

[tpapaioa]

Does this need to be in the for loop?

[tpapaioa]

The test is failing in PRT because the provider is not set up in the appliance before catalog item creation. Add the setup_provider_modscope fixture:

def catalog_item_setups(appliance, provider, setup_provider_modscope, provisioning, request, catalog, dialog):

[niyazRedhat]

Thanks for pointing the issue

[john-dupuy]

Moving to WIPTEST based on @tpapaioa's comments.

[dajoRH]

I detected some fixture changes in commit ee81aba1e6b3857889d684aa32473cdf3b492e83

The local fixture catalog_item_setups is used in the following files:

• cfme/tests/services/test_service_catalogs_bundles.py
  • test_retire_service_and_bundle_vms

Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃

[tpapaioa]

I'm not sure I understand why the fixture is yielding this group of objects. catalog_item is the last catalog item created in the loop, and is also the last entry in cat_list, so it seems redundant to include catalog_item along with the list. Also, the instantiation of vm1 and vm2 has nothing to do with creating the catalog items. I think L55-L60 would make more sense if they were in the test function, right before L143. The fixture could just yield cat_list, and references to catalog_item in the test could be cat_list[0] or cat_list[1].

Also, the f-strings f"{vm_nameX}" are equivalent to vm_nameX.

[tpapaioa]

The return value of is_succeeded() isn't being checked.

[tpapaioa]

Suggest changing this to:

catalog_items=[c.name for c in cat_list],

[jarovo]

I would rather seen this right after the catalog_item is being assigned. The append on #52 probably won't fail, but I think it is good habit to have and it may mitigate chance of inserting some code between the object creation and deletion scheduling statement.

[tpapaioa]

Looks good to me.

[valaparthvi]

Why do you need this catalog_item if catalog_item_setups already creates catalog items?

[valaparthvi]

Suggested change

	service_catalogs = ServiceCatalogs(
	appliance, catalog_bundle.catalog, catalog_bundle.name
	)
	service_catalogs.order()
	request_description = (
	f'Provisioning Service [{catalog_bundle.name}] from [{catalog_bundle.name}]'
	)
	provision_request = appliance.collections.requests.instantiate(request_description)
	provision_request = ServiceCatalogs(
	appliance, catalog_bundle.catalog, catalog_bundle.name
	).order()