[1LP][RFR] - Removing duplicate tc

[dgaikwad]

• Adding new test to check is email sent showing in-progress state or not after ordering the catalog

Purpose or Intent

PRT Run

[valaparthvi]

This test is collected twice, once for RHEV provider and once for VMWare provider. I think testing it against a single provider will be enough. And this will also resolve your need to delete the service request for a successful test execution if that is the reason why you're deleting the service request.

[valaparthvi]

Use automates instead of coverage.

[valaparthvi]

I don't see the need to delete this service request, but if you must do it, you can directly put this inside a finalize block(outside the with block) like this -

@request.addfinalizer
def _finalize():
            # Remove Request
        request_description = "Provisioning Service [{name}] from [{name}]".format(
            name=generic_catalog_item.name
        )
        service_request = appliance.collections.requests.instantiate(
            description=request_description, partial_check=True
        )
       service_request.remove_request(method="rest")

[valaparthvi]

Can you fix indentation here?

[valaparthvi]

Use f-string instead of format.

[valaparthvi]

Thanks for the changes, I still have one more request.

[valaparthvi]

Suggested change

	@pytest.mark.provider([VMwareProvider], scope="module")
	@pytest.mark.provider([InfraProvider], scope="module", selector=ONE)

This will create test like - test_xyz[virtualcenter-datastore]

The test currently has a name like test_xyz[virtualcenter-6.5-datastore]. If the name of this provider changes, the test ID and name will also change which we want to avoid as fas as possible.

[john-dupuy]

@dgaikwad I agree with @valaparthvi here, we should change this to InfraProvider

[jarovo]

There is test_provision_approval doing very similar thing.

integration_tests/cfme/tests/cloud_infra_common/test_provisioning.py

Line 152 in c4be9e7

def test_provision_approval(appliance, provider, vm_name, smtp_test, request,

[john-dupuy]

Suggested change

	@pytest.mark.meta(automate=[1380197])
	@pytest.mark.meta(automates=[1380197])

[john-dupuy]

1 small change, otherwise LGTM

[john-dupuy]

@dgaikwad I agree with @valaparthvi here, we should change this to InfraProvider

[john-dupuy]

LGTM @valaparthvi can we move this one to 1LP?

[mshriver]

As @JaryN pointed out, I think this is duplicated test coverage for something that is already automated, and this test case should not be added.

integration_tests/cfme/tests/cloud_infra_common/test_provisioning.py

Line 219 in c4be9e7

pending_from = normalize_text("{} request from {}pending approval".format(vm_type, requester))

In the existing test case, provision approval is set to manual, and we're asserting against the smtp queue that a pending approval email is sent.

This test case can be covered by simply adding the LogValidator to the existing test, after the SMTP check.

[mshriver]

This looks like its actually missing the from value? Is there a bug with CFME logging here?

[jarovo]

@mshriver I reported this issue for some infra provider at first. They fixed only for Infra and I didn't bother to verify with some other by hand, but I think meanwhile hard-coded exception for cloud providers was added. I have no idea why the person putting it there did that.

I hope after BZ 1818172 is closed, all providers will behave the same.

[jarovo]

As @JaryN pointed out, I think this is duplicated test coverage for something that is already automated, and this test case should not be added.

integration_tests/cfme/tests/cloud_infra_common/test_provisioning.py

Line 219 in c4be9e7

pending_from = normalize_text("{} request from {}pending approval".format(vm_type, requester))

In the existing test case, provision approval is set to manual, and we're asserting against the smtp queue that a pending approval email is sent.

This test case can be covered by simply adding the LogValidator to the existing test, after the SMTP check.

Note there is ongoing PR I started recently:
#10019
which is modifying the test to do some checks for request denial. If the log message really has to be checked, I can include it in the PR or make a followup.

[dgaikwad]

@mshriver, @JaryN so do we need to close this PR without merging it as we have the same test covered in another test case?

[jarovo]

@mshriver, @JaryN so do we need to close this PR without merging it as we have the same test covered in another test case?

The bug is about message not being send. Although checking log message about email sent attempt may be considered enough, we are doing more in the test I am owner of.

The test I am owner of is not making service request but just simple provisioning. I think though the service request is just some "wrapper" around normal provisioning and calls the exactly the same parts. I think it is not worthy to spend time on implementing the checking with service requests.

Also, as I am mentioning in BZ 1818172, I think the email to users are quirky in setting up as I was unable to find a way how to specify the recipient address (@dgaikwad do you know? I think it is somewhere in automate). I have doubts that they are used in real-life.

[dgaikwad]

@JaryN Sorry but I am not aware of setting up recipient address

[dgaikwad]

@JaryN , @mshriver I have updated TC as per comment please review