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.

Sign up for GitHub

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

Jump to bottom

🧪 Make pytest notify us about future warnings #15620

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from
Open

🧪 Make pytest notify us about future warnings #15620

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
912a056
🧪 Make pytest notify us about future warnings
webknjaz Nov 7, 2024
9cf4af5
Address unclosed fd warnings
AlanCoding Nov 11, 2024
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
70 changes: 42 additions & 28 deletions awx/main/tests/unit/test_tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,8 +374,8 @@
task = jobs.RunJob()
task.build_extra_vars_file(job, private_data_dir)

fd = open(os.path.join(private_data_dir, 'env', 'extravars'))
extra_vars = yaml.load(fd, Loader=SafeLoader)
with open(os.path.join(private_data_dir, 'env', 'extravars')) as fd:
extra_vars = yaml.load(fd, Loader=SafeLoader)

# ensure that strings are marked as unsafe
for name in JOB_VARIABLE_PREFIXES:
Expand All @@ -393,8 +393,8 @@

task.build_extra_vars_file(job, private_data_dir)

fd = open(os.path.join(private_data_dir, 'env', 'extravars'))
extra_vars = yaml.load(fd, Loader=SafeLoader)
with open(os.path.join(private_data_dir, 'env', 'extravars')) as fd:
extra_vars = yaml.load(fd, Loader=SafeLoader)
assert extra_vars['msg'] == self.UNSAFE
assert hasattr(extra_vars['msg'], '__UNSAFE__')

Expand All @@ -404,8 +404,8 @@

task.build_extra_vars_file(job, private_data_dir)

fd = open(os.path.join(private_data_dir, 'env', 'extravars'))
extra_vars = yaml.load(fd, Loader=SafeLoader)
with open(os.path.join(private_data_dir, 'env', 'extravars')) as fd:
extra_vars = yaml.load(fd, Loader=SafeLoader)
assert extra_vars['msg'] == {'a': [self.UNSAFE]}
assert hasattr(extra_vars['msg']['a'][0], '__UNSAFE__')

Expand All @@ -415,8 +415,8 @@

task.build_extra_vars_file(job, private_data_dir)

fd = open(os.path.join(private_data_dir, 'env', 'extravars'))
extra_vars = yaml.load(fd, Loader=SafeLoader)
with open(os.path.join(private_data_dir, 'env', 'extravars')) as fd:
extra_vars = yaml.load(fd, Loader=SafeLoader)
assert extra_vars['msg'] == self.UNSAFE
assert not hasattr(extra_vars['msg'], '__UNSAFE__')

Expand All @@ -427,8 +427,8 @@

task.build_extra_vars_file(job, private_data_dir)

fd = open(os.path.join(private_data_dir, 'env', 'extravars'))
extra_vars = yaml.load(fd, Loader=SafeLoader)
with open(os.path.join(private_data_dir, 'env', 'extravars')) as fd:
extra_vars = yaml.load(fd, Loader=SafeLoader)
assert extra_vars['msg'] == {'a': {'b': [self.UNSAFE]}}
assert not hasattr(extra_vars['msg']['a']['b'][0], '__UNSAFE__')

Expand All @@ -441,8 +441,8 @@

task.build_extra_vars_file(job, private_data_dir)

fd = open(os.path.join(private_data_dir, 'env', 'extravars'))
extra_vars = yaml.load(fd, Loader=SafeLoader)
with open(os.path.join(private_data_dir, 'env', 'extravars')) as fd:
extra_vars = yaml.load(fd, Loader=SafeLoader)
assert extra_vars['msg'] == 'other-value'
assert hasattr(extra_vars['msg'], '__UNSAFE__')

Expand All @@ -456,8 +456,8 @@

task.build_extra_vars_file(job, private_data_dir)

fd = open(os.path.join(private_data_dir, 'env', 'extravars'))
extra_vars = yaml.load(fd, Loader=SafeLoader)
with open(os.path.join(private_data_dir, 'env', 'extravars')) as fd:
extra_vars = yaml.load(fd, Loader=SafeLoader)
assert extra_vars['msg'] == self.UNSAFE
assert hasattr(extra_vars['msg'], '__UNSAFE__')

Expand Down Expand Up @@ -885,7 +885,8 @@
if verify:
assert env['K8S_AUTH_VERIFY_SSL'] == 'True'
local_path = to_host_path(env['K8S_AUTH_SSL_CA_CERT'], private_data_dir)
cert = open(local_path, 'r').read()
with open(local_path, 'r') as f:
cert = f.read()
assert cert == 'CERTDATA'
else:
assert env['K8S_AUTH_VERIFY_SSL'] == 'False'
Expand Down Expand Up @@ -936,7 +937,8 @@
credential.credential_type.inject_credential(credential, env, safe_env, [], private_data_dir)
runner_path = env[cred_env_var]
local_path = to_host_path(runner_path, private_data_dir)
json_data = json.load(open(local_path, 'rb'))
with open(local_path, 'rb') as f_host:
json_data = json.load(f_host)
assert json_data['type'] == 'service_account'
assert json_data['private_key'] == self.EXAMPLE_PRIVATE_KEY
assert json_data['client_email'] == 'bob'
Expand Down Expand Up @@ -1008,7 +1010,8 @@
credential.credential_type.inject_credential(credential, env, {}, [], private_data_dir)

config_loc = to_host_path(env['OS_CLIENT_CONFIG_FILE'], private_data_dir)
shade_config = open(config_loc, 'r').read()
with open(config_loc, 'r') as f:
shade_config = f.read()
Comment on lines +1013 to +1014
Copy link
Member Author

Choose a reason for hiding this comment

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

@AlanCoding here's an example of what I meant @ #15620 (comment)

Suggested change
with open(config_loc, 'r') as f:
shade_config = f.read()
shade_config = Path(config_loc).read_text()

assert shade_config == '\n'.join(
[
'clouds:',
Expand Down Expand Up @@ -1083,7 +1086,8 @@
assert env['ANSIBLE_NET_AUTHORIZE'] == expected_authorize
if authorize:
assert env['ANSIBLE_NET_AUTH_PASS'] == 'authorizeme'
assert open(env['ANSIBLE_NET_SSH_KEYFILE'], 'r').read() == self.EXAMPLE_PRIVATE_KEY
with open(env['ANSIBLE_NET_SSH_KEYFILE'], 'r') as f:
assert f.read() == self.EXAMPLE_PRIVATE_KEY
assert safe_env['ANSIBLE_NET_PASSWORD'] == HIDDEN_PASSWORD

def test_terraform_cloud_credentials(self, job, private_data_dir, mock_me):
Expand All @@ -1104,7 +1108,8 @@
credential.credential_type.inject_credential(credential, env, safe_env, [], private_data_dir)

local_path = to_host_path(env['TF_BACKEND_CONFIG_FILE'], private_data_dir)
config = open(local_path, 'r').read()
with open(local_path, 'r') as f:
config = f.read()
assert config == hcl_config

def test_terraform_gcs_backend_credentials(self, job, private_data_dir, mock_me):
Expand Down Expand Up @@ -1138,11 +1143,13 @@
credential.credential_type.inject_credential(credential, env, safe_env, [], private_data_dir)

local_path = to_host_path(env['TF_BACKEND_CONFIG_FILE'], private_data_dir)
config = open(local_path, 'r').read()
with open(local_path, 'r') as f:
config = f.read()
assert config == hcl_config

credentials_path = to_host_path(env['GOOGLE_BACKEND_CREDENTIALS'], private_data_dir)
credentials = open(credentials_path, 'r').read()
with open(credentials_path, 'r') as f:
credentials = f.read()
assert credentials == gce_backend_credentials

def test_custom_environment_injectors_with_jinja_syntax_error(self, private_data_dir, mock_me):
Expand Down Expand Up @@ -1351,7 +1358,8 @@
credential.credential_type.inject_credential(credential, env, {}, [], private_data_dir)

path = to_host_path(env['MY_CLOUD_INI_FILE'], private_data_dir)
assert open(path, 'r').read() == '[mycloud]\nABC123'
with open(path, 'r') as f:
assert f.read() == '[mycloud]\nABC123'

def test_custom_environment_injectors_with_unicode_content(self, private_data_dir, mock_me):
value = 'Iñtërnâtiônàlizætiøn'
Expand All @@ -1371,7 +1379,8 @@
credential.credential_type.inject_credential(credential, env, {}, [], private_data_dir)

path = to_host_path(env['MY_CLOUD_INI_FILE'], private_data_dir)
assert open(path, 'r').read() == value
with open(path, 'r') as f:
assert f.read() == value

def test_custom_environment_injectors_with_files(self, private_data_dir, mock_me):
some_cloud = CredentialType(
Expand All @@ -1391,8 +1400,10 @@

cert_path = to_host_path(env['MY_CERT_INI_FILE'], private_data_dir)
key_path = to_host_path(env['MY_KEY_INI_FILE'], private_data_dir)
assert open(cert_path, 'r').read() == '[mycert]\nCERT123'
assert open(key_path, 'r').read() == '[mykey]\nKEY123'
with open(cert_path, 'r') as f:
assert f.read() == '[mycert]\nCERT123'
with open(key_path, 'r') as f:
assert f.read() == '[mykey]\nKEY123'

def test_multi_cloud(self, private_data_dir, mock_me):
gce = CredentialType.defaults['gce']()
Expand All @@ -1415,7 +1426,8 @@

# Because this is testing a mix of multiple cloud creds, we are not going to test the GOOGLE_APPLICATION_CREDENTIALS here
path = to_host_path(env['GCE_CREDENTIALS_FILE_PATH'], private_data_dir)
json_data = json.load(open(path, 'rb'))
with open(path, 'rb') as f:
json_data = json.load(f)
assert json_data['type'] == 'service_account'
assert json_data['private_key'] == self.EXAMPLE_PRIVATE_KEY
assert json_data['client_email'] == 'bob'
Expand Down Expand Up @@ -1768,7 +1780,8 @@
credential.credential_type.inject_credential(credential, env, safe_env, [], private_data_dir)

assert env['GCE_ZONE'] == expected_gce_zone
json_data = json.load(open(env[cred_env_var], 'rb'))
with open(env[cred_env_var], 'rb') as f:
json_data = json.load(f)

Check warning on line 1784 in awx/main/tests/unit/test_tasks.py

View check run for this annotation

Codecov / codecov/patch

awx/main/tests/unit/test_tasks.py#L1783-L1784 

Added lines #L1783 - L1784 were not covered by tests
assert json_data['type'] == 'service_account'
assert json_data['private_key'] == self.EXAMPLE_PRIVATE_KEY
assert json_data['client_email'] == 'bob'
Expand Down Expand Up @@ -1797,7 +1810,8 @@
env = task.build_env(inventory_update, private_data_dir, private_data_files)

path = to_host_path(env['OS_CLIENT_CONFIG_FILE'], private_data_dir)
shade_config = open(path, 'r').read()
with open(path, 'r') as f:
shade_config = f.read()
assert (
'\n'.join(
[
Expand Down
6 changes: 5 additions & 1 deletion awx/main/utils/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1096,7 +1096,11 @@ def create_temporary_fifo(data):
path = os.path.join(tempfile.mkdtemp(), next(tempfile._get_candidate_names()))
os.mkfifo(path, stat.S_IRUSR | stat.S_IWUSR)

threading.Thread(target=lambda p, d: open(p, 'wb').write(d), args=(path, data)).start()
def tmp_write(path, data):
with open(path, 'wb') as f:
f.write(data)

threading.Thread(target=tmp_write, args=(path, data)).start()
Comment on lines +1099 to +1103
Copy link
Member Author

Choose a reason for hiding this comment

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

@AlanCoding another example of #15620 (comment):

Suggested change
def tmp_write(path, data):
with open(path, 'wb') as f:
f.write(data)
threading.Thread(target=tmp_write, args=(path, data)).start()
threading.Thread(target=lambda p, d: Path(p).write_bytes(d), args=(path, data)).start()

This is one of the reasons I didn't really want to include fixing things into the scope of the PR, since a good solution might need to be iterated on, which may make the scope grow indefinitely.

Copy link
Member

Choose a reason for hiding this comment

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

That's fine with me. I suspected Path could do this more clearly. It's your PR, you can include it or not.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't exactly expect you to merge it. I could have changed the base after your PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

@AlanCoding Ah, I was under the impression that you wanted it to be a part of the PR before merging. Anyway, it's not really blocking, I just wanted to be explicit about my observations. I think I'll add these code suggestions on top and then wait until somebody hits merge, then.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or maybe not… This import isn't there, and I won't be doing this from browser. Let's leave it as is and document somewhere that it's nice to address it.

return path


Expand Down
6 changes: 6 additions & 0 deletions awx_collection/test/awx/test_instance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@
from django.test.utils import override_settings


@pytest.mark.filterwarnings(
# FIXME: Figure out where it is emited and what causes it.
# FIXME: The suppression should be made more specific or the cause fixed.
# Ref: https://github.com/ansible/awx/pull/15620
"ignore::RuntimeWarning",
)
@pytest.mark.django_db
def test_peers_adding_and_removing(run_module, admin_user):
with override_settings(IS_K8S=True):
Expand Down
12 changes: 12 additions & 0 deletions awx_collection/test/awx/test_instance_group.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@
from awx.main.models import InstanceGroup, Instance


@pytest.mark.filterwarnings(
# FIXME: Figure out where it is emited and what causes it.
# FIXME: The suppression should be made more specific or the cause fixed.
# Ref: https://github.com/ansible/awx/pull/15620
"ignore::RuntimeWarning",
)
@pytest.mark.django_db
def test_instance_group_create(run_module, admin_user):
result = run_module(
Expand Down Expand Up @@ -36,6 +42,12 @@ def test_instance_group_create(run_module, admin_user):
assert len(all_instance_names) == 1, 'Too many instances in group {0}'.format(','.join(all_instance_names))


@pytest.mark.filterwarnings(
# FIXME: Figure out where it is emited and what causes it.
# FIXME: The suppression should be made more specific or the cause fixed.
# Ref: https://github.com/ansible/awx/pull/15620
"ignore::RuntimeWarning",
)
@pytest.mark.django_db
def test_container_group_create(run_module, admin_user, kube_credential):
pod_spec = "{ 'Nothing': True }"
Expand Down
8 changes: 8 additions & 0 deletions awx_collection/test/awx/test_workflow_job_template_node.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,14 @@
from awx.main.models import WorkflowJobTemplateNode, WorkflowJobTemplate, JobTemplate, UnifiedJobTemplate


pytestmark = pytest.mark.filterwarnings(
# FIXME: Figure out where it is emited and what causes it.
# FIXME: The suppression should be made more specific or the cause fixed.
# Ref: https://github.com/ansible/awx/pull/15620
"ignore::RuntimeWarning",
)


@pytest.fixture
def job_template(project, inventory):
return JobTemplate.objects.create(
Expand Down
Empty file removed awxkit/test/pytest.ini
View file
Open in desktop
Empty file.
4 changes: 4 additions & 0 deletions awxkit/tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,8 @@ max-line-length = 120

[pytest]
addopts = -v --tb=native

filterwarnings =
error

junit_family=xunit2
69 changes: 69 additions & 0 deletions pytest.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,75 @@ markers =
job_runtime_vars:
fixture_args:

filterwarnings =
error

# FIXME: Delete this entry once `USE_L10N` use is removed.
once:The USE_L10N setting is deprecated. Starting with Django 5.0, localized formatting of data will always be enabled. For example Django will display numbers and dates using the format of the current locale.:django.utils.deprecation.RemovedInDjango50Warning:django.conf

# FIXME: Delete this entry once `pyparsing` is updated.
once:module 'sre_constants' is deprecated:DeprecationWarning:_pytest.assertion.rewrite

# FIXME: Delete this entry once `polymorphic` is updated.
once:pkg_resources is deprecated as an API. See https.//setuptools.pypa.io/en/latest/pkg_resources.html:DeprecationWarning:_pytest.assertion.rewrite

# FIXME: Delete this entry once `zope` is updated.
once:Deprecated call to `pkg_resources.declare_namespace.'zope'.`.\nImplementing implicit namespace packages .as specified in PEP 420. is preferred to `pkg_resources.declare_namespace`. See https.//setuptools.pypa.io/en/latest/references/keywords.html#keyword-namespace-packages:DeprecationWarning:

# FIXME: Delete this entry once `coreapi` is updated.
once:'cgi' is deprecated and slated for removal in Python 3.13:DeprecationWarning:_pytest.assertion.rewrite

# FIXME: Delete this entry once the use of `distutils` is exterminated from the repo.
once:The distutils package is deprecated and slated for removal in Python 3.12. Use setuptools or check PEP 632 for potential alternatives:DeprecationWarning:_pytest.assertion.rewrite

# FIXME: Delete this entry once `coreapi` is deleted from the dependencies
# FIXME: and is no longer imported at runtime.
once:CoreAPI compatibility is deprecated and will be removed in DRF 3.17:rest_framework.RemovedInDRF317Warning:rest_framework.schemas.coreapi

# FIXME: Delete this entry once naive dates aren't passed to DB lookup
# FIXME: methods. Not sure where, might be in awx's views or in DAB.
once:DateTimeField User.date_joined received a naive datetime .2020-01-01 00.00.00. while time zone support is active.:RuntimeWarning:django.db.models.fields

# FIXME: Delete this entry once the deprecation is acted upon.
once:'index_together' is deprecated. Use 'Meta.indexes' in 'main.\w+' instead.:django.utils.deprecation.RemovedInDjango51Warning:django.db.models.options

# FIXME: Update `awx.main.migrations._dab_rbac` and delete this entry.
# once:Using QuerySet.iterator.. after prefetch_related.. without specifying chunk_size is deprecated.:django.utils.deprecation.RemovedInDjango50Warning:django.db.models.query
once:Using QuerySet.iterator.. after prefetch_related.. without specifying chunk_size is deprecated.:django.utils.deprecation.RemovedInDjango50Warning:awx.main.migrations._dab_rbac

# FIXME: Delete this entry once the **broken** always-true assertions in the
# FIXME: following tests are fixed:
# * `awx/main/tests/unit/utils/test_common.py::TestHostnameRegexValidator::test_good_call`
# * `awx/main/tests/unit/utils/test_common.py::TestHostnameRegexValidator::test_bad_call_with_inverse`
once:assertion is always true, perhaps remove parentheses\?:pytest.PytestAssertRewriteWarning:

# FIXME: Figure this out, fix and then delete the entry. It's not entirely
# FIXME: clear what emits it and where.
once:Pagination may yield inconsistent results with an unordered object_list. .class 'awx.main.models.workflow.WorkflowJobTemplateNode'. QuerySet.:django.core.paginator.UnorderedObjectListWarning:django.core.paginator

# FIXME: Figure this out, fix and then delete the entry.
once::django.core.paginator.UnorderedObjectListWarning:rest_framework.pagination

# FIXME: Replace use of `distro.linux_distribution()` via a context manager
# FIXME: in `awx/main/analytics/collectors.py` and then delete the entry.
once:distro.linux_distribution.. is deprecated. It should only be used as a compatibility shim with Python's platform.linux_distribution... Please use distro.id.., distro.version.. and distro.name.. instead.:DeprecationWarning:awx.main.analytics.collectors

# FIXME: Figure this out, fix and then delete the entry.
once:\nUsing ProtocolTypeRouter without an explicit "http" key is deprecated.\nGiven that you have not passed the "http" you likely should use Django's\nget_asgi_application...:DeprecationWarning:awx.main.routing

# FIXME: Figure this out, fix and then delete the entry.
once:Channel's inbuilt http protocol AsgiHandler is deprecated. Use Django's get_asgi_application.. instead.:DeprecationWarning:channels.routing

# FIXME: Use `codecs.open()` via a context manager
# FIXME: in `awx/main/utils/ansible.py` to close hanging file descriptors
# FIXME: and then delete the entry.
once:unclosed file <_io.BufferedReader name='[^']+'>:ResourceWarning:awx.main.utils.ansible

# FIXME: Use `open()` via a context manager
# FIXME: in `awx/main/tests/unit/test_tasks.py` to close hanging file
# FIXME: descriptors and then delete the entry.
once:unclosed file <_io.TextIOWrapper name='[^']+' mode='r' encoding='UTF-8'>:ResourceWarning:awx.main.tests.unit.test_tasks
Copy link
Member

Choose a reason for hiding this comment

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

so this line can be removed with @AlanCoding 's commit?

Copy link
Member Author

Choose a reason for hiding this comment

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

@fosterseth I think he removed one entry already, I'm not sure about others. There were several different warnings in some files. Somebody needs to check it.


# https://docs.pytest.org/en/stable/usage.html#creating-junitxml-format-files
junit_duration_report = call
# xunit1 contains more metadata than xunit2 so it's better for CI UIs:
Expand Down
Loading