-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
addFinalizer in test body #14036
addFinalizer in test body #14036
Changes from all commits
eda76b5
62e7b5e
432a6aa
b850ab9
dc61347
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -136,6 +136,13 @@ def test_positive_disable_hammer_defaults(request, function_product, target_sat) | |||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
:BZ: 1640644, 1368173 | ||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
@request.addfinalizer | ||||||||||||||||||||||||||||||||||||||
def _finalize(): | ||||||||||||||||||||||||||||||||||||||
target_sat.cli.Defaults.delete({'param-name': 'organization_id'}) | ||||||||||||||||||||||||||||||||||||||
result = target_sat.execute('hammer defaults list') | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+140
to
+143
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we move this below at least after we create defaults? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to keep this specific case as it is, as it has broken many tests in the upgrade run |
||||||||||||||||||||||||||||||||||||||
assert str(function_product.organization.id) not in result.stdout | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
target_sat.cli.Defaults.add( | ||||||||||||||||||||||||||||||||||||||
{'param-name': 'organization_id', 'param-value': function_product.organization.id} | ||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+139
to
148
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, Good catch, I've seen this behaviour earlier while writing some test, just one suggestion here, to add finalizer at least after we add entity for better readability
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if defaults adding fails but default is still being added ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Gauravtalreja1 It can be done for finalizers that touch just one thing, but as long as you change more items, you can't have them all defined before, because it would defer the point of making sure finalizer is always added. This may be odd to read, but on the other hand it also communicates that the finalizer behaves differently from the rest of the test code, meaning it is not executed first but needs to be added as early as possible |
||||||||||||||||||||||||||||||||||||||
|
@@ -154,12 +161,6 @@ def test_positive_disable_hammer_defaults(request, function_product, target_sat) | |||||||||||||||||||||||||||||||||||||
assert result.status == 0 | ||||||||||||||||||||||||||||||||||||||
assert function_product.name in result.stdout | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
@request.addfinalizer | ||||||||||||||||||||||||||||||||||||||
def _finalize(): | ||||||||||||||||||||||||||||||||||||||
target_sat.cli.Defaults.delete({'param-name': 'organization_id'}) | ||||||||||||||||||||||||||||||||||||||
result = target_sat.execute('hammer defaults list') | ||||||||||||||||||||||||||||||||||||||
assert str(function_product.organization.id) not in result.stdout | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
@pytest.mark.upgrade | ||||||||||||||||||||||||||||||||||||||
def test_positive_check_debug_log_levels(target_sat): | ||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep only one request.addfinalizer method in this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that kind of contradicts with other concerns voiced in comments, if there are more elements to remove and you want to make sure all are marked to be finalized immediately after creation but not before, then you end up with multiple finalizers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't understand this, do we really need to finalizers calling
target_sat.custom_certs_cleanup()
this specific test?Initially, I would've held the merge on this, but now that it's self-merged, could you please take care of this in a separate PR if you agree with the above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pondrejk ^