[6.15.z] Use manifester from fixture #14405
Conversation
vsedmik
changed the title
There was a problem hiding this comment.
PRT is currently down, but running the repositories test with these changes locally fails with an AttributeError:
$ pytest tests/foreman/api/test_repositories.py::test_negative_upload_expired_manifest
===================================================================================================================================== test session starts =====================================================================================================================================
*** snip ***
collected 1 item
tests/foreman/api/test_repositories.py F [100%]
========================================================================================================================================== FAILURES ===========================================================================================================================================
____________________________________________________________________________________________________________________________ test_negative_upload_expired_manifest ____________________________________________________________________________________________________________________________
*** snip ***
> manifest = function_entitlement_manifest.get_manifest()
E AttributeError: 'Response' object has no attribute 'get_manifest'
Debugging the test, we can see that function_entitlement_manifest is of type requests.models.Response, not a Manifester object:
-> manifest = function_entitlement_manifest.get_manifest()
(Pdb) type(function_entitlement_manifest)
<class 'requests.models.Response'>
(Pdb) function_entitlement_manifest.get_manifest()
*** AttributeError: 'Response' object has no attribute 'get_manifest'
As noted above, there are some test scenarios that currently require importing and instantiating Manifester in the test module. The two tests modified by this PR are examples of this type of scenario and will fail with these changes. I would therefore suggest that this PR be closed.
|
@synkd Thanks for testing. Now I get it, the problem is in the manifester context: https://github.com/SatelliteQE/manifester/blob/master/manifester/manifester.py#L421 it does not return an instance of manifester but the result of In that case it should be closed. Would you be fine if I add a marker ( |
|
@dosas You're welcome, and yes, you have the issue exactly right. I have no problem with adding the manifester marker directly to these two tests. |
|
@dosas @synkd. @synkd is correct. Calling |
| @@ -199,10 +198,9 @@ def test_negative_upload_expired_manifest(module_org, target_sat): | |||
|
|
|||
| :expectedresults: Manifest refresh should fail and return error message | |||
| """ | |||
| manifester = Manifester(manifest_category=settings.manifest.entitlement) | |||
| manifest = manifester.get_manifest() | |||
| manifest = function_entitlement_manifest.get_manifest() | |||
There was a problem hiding this comment.
@dosas As you've mentioned it does not return an instance of manifester but the result of get_manifest(), so what if we can just use function_entitlement_manifest for manifest?
CC @synkd @jyejare wdyt?
| manifest = function_entitlement_manifest.get_manifest() | |
| target_sat.upload_manifest(module_org.id, function_entitlement_manifest.content) | |
| request.addfinalizer(function_entitlement_manifest.delete_subscription_allocation) |
There was a problem hiding this comment.
@Gauravtalreja1 This would be technically possible, but currently, it's a bit simpler to generate the manifest from a local Manifester instance. Manifester's delete_subscription_allocation method does allow passing in an arbitrary UUID argument for deletion [1], but the allocation UUID is not immediately accessible from the function_entitlement_manifest object. It's embedded in function_entitlement_manifest.url but would need to be extracted from a URL path like this one [2].
With that said, it should be a fairly simple change to add a line like the following to the end of Manifester's trigger_manifest_export method [3]:
manifest.uuid = self.allocation_uuid
The UUID would then be accessible as function_entitlement_manifest.uuid, which could be passed to Manifester.delete_subscription_allocation(). From the looks of it, this same change would also enable tests/upgrades/test_subscription.py::test_post_subscription_auto_attach to use a Manifester fixture.
If this is the direction that people want to go, then I'll also note that I would welcome a community contribution to Manifester making the necessary change.
There was a problem hiding this comment.
@synkd I can continue here once a new release of manifester is out
There was a problem hiding this comment.
@dosas I just cut a new Manifester release that includes this change. However, the build/publish to PyPi job failed, and I will be OOO the rest of this week and next week. We'll need to hold off on merging this change until the new Manifester release is published, which may need to wait until I return.
There was a problem hiding this comment.
@synkd hello, mind revisiting this? it's been sitting here for a while. Thanks!
|
@synkd Are you okay with these changes? |
|
Not relevant anymore |
#14377
We should check with the authors of these tests, but I believe it was by design that they did not use Manifester via fixtures in these cases. function_entitlement_manifest returns a Request object, not a Manifester object, so the Manifester instance methods are not available to it or the other Manifester fixtures. If a test requires Manifester instance methods, it's currently necessary to instantiate Manifester locally in order to access those methods.
@omkarkhatavkar @ColeHiggins2 can you confirm that these tests are intentionally instantiating Manifester objects?