Skip to content
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

Merged
merged 5 commits into from
Mar 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions tests/foreman/api/test_computeresource_libvirt.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,9 @@ def test_positive_create_with_name_description(
location=[module_location],
url=LIBVIRT_URL,
).create()
request.addfinalizer(compresource.delete)
assert compresource.name == name
assert compresource.description == name
request.addfinalizer(compresource.delete)


@pytest.mark.tier2
Expand All @@ -134,9 +134,9 @@ def test_positive_create_with_orgs_and_locs(request, module_target_sat):
compresource = module_target_sat.api.LibvirtComputeResource(
location=locs, organization=orgs, url=LIBVIRT_URL
).create()
request.addfinalizer(compresource.delete)
assert {org.name for org in orgs} == {org.read().name for org in compresource.organization}
assert {loc.name for loc in locs} == {loc.read().name for loc in compresource.location}
request.addfinalizer(compresource.delete)


@pytest.mark.tier2
Expand Down Expand Up @@ -175,8 +175,8 @@ def test_negative_create_with_same_name(request, module_target_sat, module_org,
cr = module_target_sat.api.LibvirtComputeResource(
location=[module_location], name=name, organization=[module_org], url=LIBVIRT_URL
).create()
assert cr.name == name
request.addfinalizer(cr.delete)
assert cr.name == name
with pytest.raises(HTTPError):
module_target_sat.api.LibvirtComputeResource(
name=name,
Expand Down Expand Up @@ -245,19 +245,16 @@ def test_negative_update_same_name(request, module_target_sat, module_org, modul
compresource = module_target_sat.api.LibvirtComputeResource(
location=[module_location], name=name, organization=[module_org], url=LIBVIRT_URL
).create()
request.addfinalizer(compresource.delete)
new_compresource = module_target_sat.api.LibvirtComputeResource(
location=[module_location], organization=[module_org], url=LIBVIRT_URL
).create()
request.addfinalizer(new_compresource.delete)
new_compresource.name = name
with pytest.raises(HTTPError):
new_compresource.update(['name'])
assert new_compresource.read().name != name

@request.addfinalizer
def _finalize():
compresource.delete()
new_compresource.delete()


@pytest.mark.tier2
@pytest.mark.parametrize('url', **parametrized({'random': gen_string('alpha'), 'empty': ''}))
Expand Down
13 changes: 4 additions & 9 deletions tests/foreman/api/test_discoveryrule.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,11 @@ def test_positive_multi_provision_with_rule_limit(

:CaseImportance: High
"""

discovered_host1 = module_target_sat.api_factory.create_discovered_host()
request.addfinalizer(module_target_sat.api.Host(id=discovered_host1['id']).delete)
discovered_host2 = module_target_sat.api_factory.create_discovered_host()
request.addfinalizer(module_target_sat.api.DiscoveredHost(id=discovered_host2['id']).delete)
rule = module_target_sat.api.DiscoveryRule(
max_count=1,
hostgroup=module_discovery_hostgroup,
Expand All @@ -181,14 +184,6 @@ def test_positive_multi_provision_with_rule_limit(
organization=[discovery_org],
priority=1000,
).create()
request.addfinalizer(rule.delete)
result = module_target_sat.api.DiscoveredHost().auto_provision_all()
assert '1 discovered hosts were provisioned' in result['message']

# Delete discovery rule
@request.addfinalizer
def _finalize():
rule.delete()
module_target_sat.api.Host(id=discovered_host1['id']).delete()
module_target_sat.api.DiscoveredHost(id=discovered_host2['id']).delete()
with pytest.raises(HTTPError):
rule.read()
5 changes: 5 additions & 0 deletions tests/foreman/api/test_http_proxy.py
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,11 @@ def test_positive_sync_proxy_with_certificate(request, target_sat, module_org, m

:customerscenario: true
"""

@request.addfinalizer
def _finalize():
target_sat.custom_certs_cleanup()
Comment on lines +306 to +309
Copy link
Collaborator

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?

Copy link
Contributor Author

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

Copy link
Collaborator

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


# Cleanup any existing certs that may conflict
target_sat.custom_certs_cleanup()
proxy_host = settings.http_proxy.auth_proxy_url.replace('http://', '').replace(':3128', '')
Expand Down
9 changes: 3 additions & 6 deletions tests/foreman/api/test_parameters.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@ def test_positive_parameter_precedence_impact(
param_value = gen_string('alpha')

cp = module_target_sat.api.CommonParameter(name=param_name, value=param_value).create()
request.addfinalizer(cp.delete)
host = module_target_sat.api.Host(organization=module_org, location=module_location).create()
request.addfinalizer(host.delete)
result = [res for res in host.all_parameters if res['name'] == param_name]
assert result[0]['name'] == param_name
assert result[0]['associated_type'] == 'global'
Expand All @@ -48,19 +50,14 @@ def test_positive_parameter_precedence_impact(
organization=[module_org],
group_parameters_attributes=[{'name': param_name, 'value': param_value}],
).create()
request.addfinalizer(hg.delete)
host.hostgroup = hg
host = host.update(['hostgroup'])
result = [res for res in host.all_parameters if res['name'] == param_name]
assert result[0]['name'] == param_name
assert result[0]['associated_type'] != 'global'
assert result[0]['associated_type'] == 'host group'

@request.addfinalizer
def _finalize():
host.delete()
hg.delete()
cp.delete()

host.host_parameters_attributes = [{'name': param_name, 'value': param_value}]
host = host.update(['host_parameters_attributes'])
result = [res for res in host.all_parameters if res['name'] == param_name]
Expand Down
2 changes: 1 addition & 1 deletion tests/foreman/cli/test_classparameters.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ def test_positive_list(
location=module_puppet_loc.id,
environment=module_puppet['env'].name,
).create()
host.add_puppetclass(data={'puppetclass_id': module_puppet['class']['id']})
request.addfinalizer(host.delete)
host.add_puppetclass(data={'puppetclass_id': module_puppet['class']['id']})
hostgroup = session_puppet_enabled_sat.cli_factory.hostgroup(
{
'puppet-environment-id': module_puppet['env'].id,
Expand Down
2 changes: 1 addition & 1 deletion tests/foreman/cli/test_computeresource_osp.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ def test_crud_and_duplicate_name(self, request, id_type, osp_version, target_sat
'url': osp_version,
}
)
request.addfinalizer(lambda: self.cr_cleanup(compute_resource['id'], id_type, target_sat))
assert compute_resource['name'] == name
assert target_sat.cli.ComputeResource.exists(search=(id_type, compute_resource[id_type]))

Expand All @@ -102,7 +103,6 @@ def test_crud_and_duplicate_name(self, request, id_type, osp_version, target_sat
else:
compute_resource = target_sat.cli.ComputeResource.info({'id': compute_resource['id']})
assert new_name == compute_resource['name']
request.addfinalizer(lambda: self.cr_cleanup(compute_resource['id'], id_type, target_sat))

@pytest.mark.tier3
def test_negative_create_osp_with_url(self, target_sat):
Expand Down
8 changes: 2 additions & 6 deletions tests/foreman/cli/test_discoveredhost.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,8 @@ def test_rhel_pxe_discovery_provisioning(

assert 'Host created' in result[0]['message']
host = sat.api.Host().search(query={"search": f'id={discovered_host.id}'})[0]
assert host

# teardown
request.addfinalizer(lambda: sat.provisioning_cleanup(host.name))
assert host

wait_for(
lambda: host.read().build_status_label != 'Pending installation',
Expand Down Expand Up @@ -131,10 +129,8 @@ def test_rhel_pxeless_discovery_provisioning(
)
assert 'Host created' in result[0]['message']
host = sat.api.Host().search(query={"search": f'id={discovered_host.id}'})[0]
assert host

# teardown
request.addfinalizer(lambda: sat.provisioning_cleanup(host.name))
assert host

wait_for(
lambda: host.read().build_status_label != 'Pending installation',
Expand Down
27 changes: 8 additions & 19 deletions tests/foreman/cli/test_errata.py
Original file line number Diff line number Diff line change
Expand Up @@ -688,16 +688,11 @@ def test_positive_list_affected_chosts_by_erratum_restrict_flag(
'inclusion': 'false',
}
)

@request.addfinalizer
def cleanup():
cv_filter_cleanup(
target_sat,
cv_filter['filter-id'],
module_cv,
module_sca_manifest_org,
module_lce,
request.addfinalizer(
pondrejk marked this conversation as resolved.
Show resolved Hide resolved
lambda: cv_filter_cleanup(
target_sat, cv_filter['filter-id'], module_cv, module_sca_manifest_org, module_lce
)
)

# Make rule to hide the RPM that creates the need for the installable erratum
target_sat.cli_factory.content_view_filter_rule(
Expand Down Expand Up @@ -861,17 +856,11 @@ def test_host_errata_search_commands(
'inclusion': 'false',
}
)

@request.addfinalizer
def cleanup():
cv_filter_cleanup(
target_sat,
cv_filter['filter-id'],
module_cv,
module_sca_manifest_org,
module_lce,
request.addfinalizer(
pondrejk marked this conversation as resolved.
Show resolved Hide resolved
lambda: cv_filter_cleanup(
target_sat, cv_filter['filter-id'], module_cv, module_sca_manifest_org, module_lce
)

)
# Make rule to exclude the specified bugfix package
target_sat.cli_factory.content_view_filter_rule(
{
Expand Down
13 changes: 7 additions & 6 deletions tests/foreman/cli/test_hammer.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this below at least after we create defaults?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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
@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

Copy link
Contributor

@lpramuk lpramuk Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Expand All @@ -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):
Expand Down
2 changes: 1 addition & 1 deletion tests/foreman/cli/test_satellitesync.py
Original file line number Diff line number Diff line change
Expand Up @@ -2272,6 +2272,7 @@ def test_positive_custom_cdn_with_credential(
meta_file = 'metadata.json'
crt_file = 'source.crt'
pub_dir = '/var/www/html/pub/repos'
request.addfinalizer(lambda: target_sat.execute(f'rm -rf {pub_dir}'))

# Export the repository in syncable format and move it
# to /var/www/html/pub/repos to mimic custom CDN.
Expand All @@ -2288,7 +2289,6 @@ def test_positive_custom_cdn_with_credential(
exp_dir = exp_dir[0].replace(meta_file, '')

assert target_sat.execute(f'mv {exp_dir} {pub_dir}').status == 0
request.addfinalizer(lambda: target_sat.execute(f'rm -rf {pub_dir}'))
target_sat.execute(f'semanage fcontext -a -t httpd_sys_content_t "{pub_dir}(/.*)?"')
target_sat.execute(f'restorecon -R {pub_dir}')

Expand Down
4 changes: 2 additions & 2 deletions tests/foreman/cli/test_subnet.py
Original file line number Diff line number Diff line change
Expand Up @@ -199,8 +199,8 @@ def test_negative_update_attributes(request, options, module_target_sat):
:CaseImportance: Medium
"""
subnet = module_target_sat.cli_factory.make_subnet()
options['id'] = subnet['id']
request.addfinalizer(lambda: module_target_sat.cli.Subnet.delete({'id': subnet['id']}))
options['id'] = subnet['id']
with pytest.raises(CLIReturnCodeError, match='Could not update the subnet:'):
module_target_sat.cli.Subnet.update(options)
# check - subnet is not updated
Expand All @@ -223,8 +223,8 @@ def test_negative_update_address_pool(request, options, module_target_sat):
:CaseImportance: Medium
"""
subnet = module_target_sat.cli_factory.make_subnet()
opts = {'id': subnet['id']}
request.addfinalizer(lambda: module_target_sat.cli.Subnet.delete({'id': subnet['id']}))
opts = {'id': subnet['id']}
# generate pool range from network address
for key, val in options.items():
opts[key] = re.sub(r'\d+$', str(val), subnet['network-addr'])
Expand Down
8 changes: 4 additions & 4 deletions tests/foreman/destructive/test_capsule_loadbalancer.py
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ def test_loadbalancer_install_package(
registration.

"""

# Register content host
result = rhel7_contenthost.register(
org=module_org,
Expand Down Expand Up @@ -219,6 +220,9 @@ def test_loadbalancer_install_package(
if loadbalancer_setup['setup_capsules']['capsule_1'].hostname in result.stdout
else loadbalancer_setup['setup_capsules']['capsule_2']
)
request.addfinalizer(
lambda: registered_to_capsule.power_control(state=VmState.RUNNING, ensure=True)
)

# Remove the packages from the client
result = rhel7_contenthost.execute('yum remove -y tree')
Expand All @@ -231,10 +235,6 @@ def test_loadbalancer_install_package(
result = rhel7_contenthost.execute('yum install -y tree')
assert result.status == 0

@request.addfinalizer
def _finalize():
registered_to_capsule.power_control(state=VmState.RUNNING, ensure=True)


@pytest.mark.rhel_ver_match('[^6]')
@pytest.mark.tier1
Expand Down
Loading
Loading