[RFR] Test provision approval deny #10019
Conversation
jarovo
changed the title
john-dupuy
added
the
test-cleanup
| """ | ||
| pass | ||
| def check_subjects(): |
There was a problem hiding this comment.
Optional since this is an internal method
| def check_subjects(): | |
| def _check_subjects(): |
There was a problem hiding this comment.
yeah it is internal -- not really accessible from outside and I think that makes clear it is "private-ish" in Java terms. So I am not sure what the underscore helps with, but I have no problems to change it.
There was a problem hiding this comment.
It is just preference for me, which is why I made it optional. Though in python there are no truly private methods, I still think it is good practice to mark internal/private methods with the pre-pended _
| for subject in expected_subjects: | ||
| if not any(subject in r_sub for r_sub in subjects): | ||
| logger.info('Subject {subjects} not found in the received emails yet.') | ||
| return False | ||
| logger.info('All expected subjects found in the received email list.') | ||
| return True |
There was a problem hiding this comment.
Q: Could you do?
| for subject in expected_subjects: | |
| if not any(subject in r_sub for r_sub in subjects): | |
| logger.info('Subject {subjects} not found in the received emails yet.') | |
| return False | |
| logger.info('All expected subjects found in the received email list.') | |
| return True | |
| return subjects in expected_subjects |
There was a problem hiding this comment.
From what you have now, it looks like subjects and expected_subjects are not exactly of the same structure, is there someway you could enforce that to simplify the logic of this method?
There was a problem hiding this comment.
From what you have now, it looks like
subjectsandexpected_subjectsare not exactly of the same structure, is there someway you could enforce that to simplify the logic of this method?
I was thinking about the same... the code looks strange as it is and I though why it is not written the way as you suggest. I am quite sure the reason is that we need to check the subject to be just a substring of the strings in expected_subjects. Note "foo" in "barfoobar" is True.
Perhaps it is something worthy to check and comment on it in the code or change the code for this to be more clear.
Note I also discovered that the smtp_test "mock" provides a subject_like argument, but I figured out I cannot use it and keep the behaviour same enough for my liking.
There was a problem hiding this comment.
I see, I think a comment explaining this will suffice for now. Then we can evaluate whether to cleanup this logic a bit later.
| provision_request = appliance.collections.requests.instantiate(cells=cells) | ||
| if edit: | ||
|
|
||
| def action_edit(): |
There was a problem hiding this comment.
To keep the test structure more transparent, I think it'd be helpful if these action_ function were defined out of the test case itself. What do you think?
There was a problem hiding this comment.
I am not sure what is better but I have not much against putting them out except that I will surely need to pass more arguments to them.
I kept those actions_* inside as I didn't think they are reusable anywhere else.
There was a problem hiding this comment.
Okay, I'm fine with keeping them in the test function, I'd consider my comment optional
There was a problem hiding this comment.
Agreed @john-dupuy, but I think this is optional to this PR.
This adds checking denying of provisioning request. The code was not written in compact way so I did some generalization of the code for checking emails. It is now not waiting for some count of emails but just waits until all expected subjects are present.
For checking different approval actions I used something like strategy design pattern.
Looking at the PR lines stats... removing 98 lines and adding 67 while adding tests ... Is this not awesome?
{{pytest: cfme/tests/cloud_infra_common/test_provisioning.py::test_provision_approval --use-provider complete --long-running -v}}
local results
There are networking/container issues with prt. thus I had to run the 9d06718 locally. All passed: