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 1 commit
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
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
84 changes: 84 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,90 @@ 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.


# 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.BufferedReader name='[^']+'>:ResourceWarning:awx.main.tests.unit.test_tasks

# 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:_pytest.python

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

Choose a reason for hiding this comment

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

This feels like re-arranging deck chairs on the Titanic. Great that your change gets us exposed to new warnings, but it will make dependency upgrades take longer, which already happen too infrequently, and it almost guarantees that none of these issues will be worked. And they really should be.

The methods here are fine, but I'd like to at least have at least a plan to knock out the borderline absurd ones in the list.

Copy link
Member Author

Choose a reason for hiding this comment

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

I skipped addressing them because that wasn't my goal. I wanted to show how to configure pytest properly. Fixing them can be planned for whenever tech debt is worked on.
I don't think that it causes more work, it just shifts the time when mandatory changes need to be made. Besides, you can always add more ignores if you want to postpone.

Copy link
Member

Choose a reason for hiding this comment

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

Is this PR, itself, not tech debt? And like I said, I'm fine to file followup, but "whenever tech debt is worked on" doesn't happen. Even a catch-all issue referencing your list will never get attention.

I meant it to comment on the warning about hanging file descriptors because absolutely nobody will disagree with that change, for example:

awx/awx/asgi.py

Lines 27 to 28 in 69baa73

fd = open("/var/lib/awx/.tower_version", "r")
if fd.read().strip() != tower_version:

I don't want to just have this added to a forever-ignore list, unless, for some reason I'm wrong about the complexity of it. Again, filing issues about the specific issues being ignored might be a workable approach.

Copy link
Member Author

@webknjaz webknjaz Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@AlanCoding I see that, but this is as much as I'm willing to contribute right now. So merging it already would be more useful than waiting for months to see if I'll ever get to it.

This PR is useful in that it would prevent new similar problems from appearing silently over time. That's what I want out of it. This patch is not a tech debt, but rather documenting the existing issues that need to be addressed regardless of whether the strictness setting is set, though.

Copy link
Member

Choose a reason for hiding this comment

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

This doesn't just document existing issues, it changes the way of working by introducing an ignore-list. The reason the warnings were not silenced is because of a shared belief that they are important and should have code changes to resolve. This disposition doesn't necessarily change... but if you're not clear, people will do the wrong thing. I don't trust that without an example, so I created one:

webknjaz#2

Getting a new process for contributions, without getting some contributions (separate PRs is fine) is not something I'm really happy with. But here, I'm seeking an explicit consensus between you and me, and specific followup issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, that makes sense in a way. Though, I don't think it changes the process. Previously, deprecation warnings would have to be addressed at some point and they were invisible (by default UserWarning instances are printed on the terminal, while many things, DeprecationWarning and FutureDeprecationWarning included never show up anywhere). To reveal those, one has to know upfront that they have to invoke everything using python -Werror -Im thing (or similar via an env var). And then, those places would have to be fixed. The problem is that with invisible warnings that accumulate over time, one would have to be aware of all these. And the process was fixing the errors too late when upstreams would replace warnings with hard failures.
The current setup makes these things visible ahead of time which is a pretty much standard best practice in the external Python world. So to me saying that the process changed would be too strong. I'd rather say that it became more transparent.

Copy link
Member

Choose a reason for hiding this comment

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

Remaining easily workable categories I see are:

  • FIXME: Delete this entry once USE_L10N use is removed
  • remove distutils from repo (only use, not dep)
  • Delete this entry once naive dates aren't passed to DB lookup
  • 'index_together' is deprecated. Use 'Meta.indexes'
  • Using QuerySet.iterator.. after prefetch_related.. without specifying chunk_size
  • remove always-true assertions
  • fix Pagination may yield inconsistent results with an unordered object_list. .class
  • Channel's inbuilt http protocol AsgiHandler is deprecated. Use Django's get_asgi_application
  • Use codecs.open() via a context manager

I would like these to be filed, so @djyasin can have the available for the next sprint. I would have liked to have them even for story pointing, but it might be too late for the current round.

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 fair. I was also postponing fixing things like open() under CM, since those are straightforward and could be a good learning opportunity for folks to try addressing them as opposed to more involved problems.

Copy link
Member Author

Choose a reason for hiding this comment

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

By the way, in the linked place in asgi, a more elegant solution would be pathlib.Path('/var/lib/awx/.tower_version').read_text().strip() even. This is to say that every place needs to be inspected additionally, beyond the generic suggestion I've put in the comments.


# 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