addFinalizer in test body

[pondrejk]

Problem Statement

Turns out that @request.addFinalizer when used in test body (not in fixture) is dependent on the execution order, so basically when some assertion defined in the test before the finalizer part fails, finalizer is not added and so not executed.

This exactly happened in test_positive_disable_hammer_defaults in upgrades, breaking a lot of other tests in the same run. Big kudos to @lpramuk for isolating the issue there.

Solution

Moving the addFinalizer part to the start of the test, fixes the problem. Prt run won't reveal much in this case, but running the previous version of the test with induced assertion error leaves satellite with:

~]# hammer defaults list
----------------|------
PARAMETER       | VALUE
----------------|------
organization_id | 41   
----------------|------

...which means that the finalizer didn't run. With this pr, the defaults are properly cleaned up:

~]# hammer defaults list
----------|------
PARAMETER | VALUE
----------|------

Related Issues

I did a quick search for this pattern across robottelo and I found quite a bit of cases where @request.addfinalizer is added towards the end of the test body, with some preceding assertions that could possibly break it:

• robottelo/tests/foreman/cli/test_errata.py

  Line 692 in d9c5ea8

  @request.addfinalizer

• robottelo/tests/foreman/api/test_discoveryrule.py

  Line 188 in d9c5ea8

  @request.addfinalizer

• robottelo/tests/foreman/api/test_computeresource_libvirt.py

  Line 256 in d9c5ea8

  @request.addfinalizer

• robottelo/tests/foreman/api/test_parameters.py

  Line 58 in d9c5ea8

  @request.addfinalizer

• robottelo/tests/foreman/maintain/test_maintenance_mode.py

  Line 146 in d9c5ea8

  @request.addfinalizer

• robottelo/tests/foreman/api/test_http_proxy.py

  Line 316 in d9c5ea8

  @request.addfinalizer

• robottelo/tests/foreman/maintain/test_packages.py

  Line 219 in d9c5ea8

  @request.addfinalizer

• robottelo/tests/foreman/ui/test_computeresource_gce.py

  Line 217 in d9c5ea8

  @request.addfinalizer

• robottelo/tests/foreman/maintain/test_advanced.py

  Line 104 in d9c5ea8

  @request.addfinalizer

• robottelo/tests/foreman/destructive/test_capsule_loadbalancer.py

  Line 234 in d9c5ea8

  @request.addfinalizer

• robottelo/tests/foreman/maintain/test_health.py

  Line 219 in d9c5ea8

  @request.addfinalizer

• robottelo/tests/foreman/ui/test_ansible.py

  Line 233 in d9c5ea8

  @request.addfinalizer

This doesn't include request.addFinalizer(...) calls that likely suffer from the same problem. Now, I could go and reorder all those occurrences, but I feel we should discuss this team wide and maybe abolish this practice all together

[Griffin-Sullivan]

I see it's on the agenda for tomorrow, but yeah each team will need to fix these.

[lpramuk]

LGTM
I've enjoyed collab with endeavour team. Finding this problematic test by bisecting test collection 😉

[Gauravtalreja1]

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

	@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
	target_sat.cli.Defaults.add(
	{'param-name': 'organization_id', 'param-value': function_product.organization.id}
	)
	target_sat.cli.Defaults.add(
	{'param-name': 'organization_id', 'param-value': function_product.organization.id}
	)
	@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

[lpramuk]

What if defaults adding fails but default is still being added ?
So I like finalizer first

[pondrejk]

@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

[pondrejk]

I'm drafting this so that I could fix other occurrences as discussed on the meeting today

[Gauravtalreja1]

Can we move it at least below after variables get defined? functionally I think it wouldn't affect as we're checking if host exists here first before delete

[JacobCallahan]

What would happen if the finalizer here was called before one of those arguments was created, cv_filter for example?

[pondrejk]

@JacobCallahan yeah it is a confusing syntax compared to what we are normally used to in python. The function marked with @request.addfinalizer within the test function is executed after the test runs, so you can use variables even before you define them (!). But at the same time, the position of the block within the test determines when the finalizer is added. So I place these blocks at the start to be sure the finalizer is added at all times. The position in test matters for addition but not for execution

[lhellebr]

And I think that's what Jacob asks about. What if the finalizer is added (here, at the start) and then something fails after it is added, but before e.g. cv_filter variable is defined. I suppose the finalizer would throw an exception?

[JacobCallahan]

correct @lhellebr. at that point, you'd raise an exception because the name cv_filter is referenced before definition. Moving this to right after everything is defined is safer.

[pondrejk]

@JacobCallahan @lhellebr I see, yeah I suppose it would raise an exception, but that's not different from "normal" finalizer behavior if defined in fixture. Consider the two alternatives when having finalizer in test body:

a) tests fails in the middle, finalizer added before -> result: finalizer added and executed, exception raised if referencing stuff that didn't get created
b) test fails in the middle, finalizer added after -> result: finalizer not added nor executed, not cleaning up anything that got created before failure, possibly causing interference with other tests

Situation (b) actually happens in the upgrade run, and it took considerable time and effort just to find out what is going on. Therefore I'd much rather have situation (a), which would be easier to debug

[JacobCallahan]

same question as before, and for any other similar

[JacobCallahan]

similar question as before. if the finalizer is called before sync_plans is populated (with at least two entries), then this will error.

[pondrejk]

@Gauravtalreja1 @JacobCallahan @lhellebr hello, based on your feedback I changed ordering where it made sense. Could you please revisit?

[JacobCallahan]

ACK. Thanks for revisiting this @pondrejk. As long as we're aware of the name error risk with ordering, I'm fine with the changes.

[Gauravtalreja1]

Can we keep only one request.addfinalizer method in this test?

[pondrejk]

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

[Gauravtalreja1]

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?

[Gauravtalreja1]

@pondrejk ^

[lhellebr]

This is already merged but I discussed this with Peter and I ACK the final result where the order of actions makes more sense.