Deprecation Warnings #6256
Deprecation Warnings #6256
Conversation
|
You are changing one of the avocado utils which has already been migrated to AAutils project https://github.com/avocado-framework/aautils and this utility will be removed after the LTS release. Please make sure that all your proposed changes are already in AAutils and this PR is only backport. For more information about AAutlis migration see https://avocado-framework.readthedocs.io/en/latest/blueprints/BP005.html For a list of migrated utilities see https://avocado-framework.github.io/aautils.html |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR expands the GitHub Actions workflow trigger to monitor additional utility file paths, and adds deprecation warnings to six utility modules (ar, astring, gdb, process, script, wait). Each module imports log_deprecation and emits a warning at import time with its module name. No functional changes are made to existing logic; only import-time side effects are introduced. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds deprecation warnings to several modules in avocado/utils, which aligns with the PR's title and description. The method used, while functional, deviates from standard Python practices for issuing deprecation warnings. I've added comments with suggestions to use the built-in warnings module, which is more idiomatic.
Additionally, this PR introduces a new test file selftests/functional/utils/genio.py. While the tests themselves are well-written and valuable, they seem out of scope for a pull request focused on deprecation warnings. To maintain a clean and atomic commit history, it would be better to move the addition of these tests to a separate pull request.
| # pylint: disable=wrong-import-position | ||
| from avocado.utils.deprecation import log_deprecation | ||
|
|
||
| log_deprecation.warning("ar") |
There was a problem hiding this comment.
Using a custom log_deprecation object with a bottom-of-file import for deprecation warnings is a possible pattern, but it deviates from standard Python practices. The built-in warnings module is the idiomatic way to handle deprecations.
Using warnings.warn() would:
- Adhere to PEP 8 by allowing the import to be at the top of the file.
- Provide users with standard mechanisms to filter or handle these warnings (e.g., via
warnings.simplefilteror the-Wcommand-line option). - Avoid the need for a custom implementation and a
flush()call.
Consider replacing this with a standard DeprecationWarning at the top of the module. For example:
# at the top of avocado/utils/ar.py
import warnings
warnings.warn(
"The 'ar' utility is deprecated. It has been moved to the AAutils "
"project and will be removed after the next LTS release. For more "
"information, please see: https://github.com/avocado-framework/aautils",
DeprecationWarning,
stacklevel=2
)If the custom log_deprecation is required for project-specific logging aggregation, please disregard this suggestion.
References
- PEP 8 recommends that imports should be at the top of the file. The current implementation places the import at the end of the file, which is a deviation from this standard. (link)
| # pylint: disable=wrong-import-position | ||
| from avocado.utils.deprecation import log_deprecation | ||
|
|
||
| log_deprecation.warning("astring") |
There was a problem hiding this comment.
Using a custom log_deprecation object with a bottom-of-file import for deprecation warnings is a possible pattern, but it deviates from standard Python practices. The built-in warnings module is the idiomatic way to handle deprecations.
Using warnings.warn() would:
- Adhere to PEP 8 by allowing the import to be at the top of the file.
- Provide users with standard mechanisms to filter or handle these warnings (e.g., via
warnings.simplefilteror the-Wcommand-line option). - Avoid the need for a custom implementation and a
flush()call.
Consider replacing this with a standard DeprecationWarning at the top of the module. For example:
# at the top of avocado/utils/astring.py
import warnings
warnings.warn(
"The 'astring' utility is deprecated. It has been moved to the AAutils "
"project and will be removed after the next LTS release. For more "
"information, please see: https://github.com/avocado-framework/aautils",
DeprecationWarning,
stacklevel=2
)If the custom log_deprecation is required for project-specific logging aggregation, please disregard this suggestion.
References
- PEP 8 recommends that imports should be at the top of the file. The current implementation places the import at the end of the file, which is a deviation from this standard. (link)
| # pylint: disable=wrong-import-position | ||
| from avocado.utils.deprecation import log_deprecation | ||
|
|
||
| log_deprecation.warning("gdb") |
There was a problem hiding this comment.
Using a custom log_deprecation object with a bottom-of-file import for deprecation warnings is a possible pattern, but it deviates from standard Python practices. The built-in warnings module is the idiomatic way to handle deprecations.
Using warnings.warn() would:
- Adhere to PEP 8 by allowing the import to be at the top of the file.
- Provide users with standard mechanisms to filter or handle these warnings (e.g., via
warnings.simplefilteror the-Wcommand-line option). - Avoid the need for a custom implementation and a
flush()call.
Consider replacing this with a standard DeprecationWarning at the top of the module. For example:
# at the top of avocado/utils/gdb.py
import warnings
warnings.warn(
"The 'gdb' utility is deprecated. It has been moved to the AAutils "
"project and will be removed after the next LTS release. For more "
"information, please see: https://github.com/avocado-framework/aautils",
DeprecationWarning,
stacklevel=2
)If the custom log_deprecation is required for project-specific logging aggregation, please disregard this suggestion.
References
- PEP 8 recommends that imports should be at the top of the file. The current implementation places the import at the end of the file, which is a deviation from this standard. (link)
| # pylint: disable=wrong-import-position | ||
| from avocado.utils.deprecation import log_deprecation | ||
|
|
||
| log_deprecation.warning("process") |
There was a problem hiding this comment.
Using a custom log_deprecation object with a bottom-of-file import for deprecation warnings is a possible pattern, but it deviates from standard Python practices. The built-in warnings module is the idiomatic way to handle deprecations.
Using warnings.warn() would:
- Adhere to PEP 8 by allowing the import to be at the top of the file.
- Provide users with standard mechanisms to filter or handle these warnings (e.g., via
warnings.simplefilteror the-Wcommand-line option). - Avoid the need for a custom implementation and a
flush()call.
Consider replacing this with a standard DeprecationWarning at the top of the module. For example:
# at the top of avocado/utils/process.py
import warnings
warnings.warn(
"The 'process' utility is deprecated. It has been moved to the AAutils "
"project and will be removed after the next LTS release. For more "
"information, please see: https://github.com/avocado-framework/aautils",
DeprecationWarning,
stacklevel=2
)If the custom log_deprecation is required for project-specific logging aggregation, please disregard this suggestion.
References
- PEP 8 recommends that imports should be at the top of the file. The current implementation places the import at the end of the file, which is a deviation from this standard. (link)
| # pylint: disable=wrong-import-position | ||
| from avocado.utils.deprecation import log_deprecation | ||
|
|
||
| log_deprecation.warning("script") |
There was a problem hiding this comment.
Using a custom log_deprecation object with a bottom-of-file import for deprecation warnings is a possible pattern, but it deviates from standard Python practices. The built-in warnings module is the idiomatic way to handle deprecations.
Using warnings.warn() would:
- Adhere to PEP 8 by allowing the import to be at the top of the file.
- Provide users with standard mechanisms to filter or handle these warnings (e.g., via
warnings.simplefilteror the-Wcommand-line option). - Avoid the need for a custom implementation and a
flush()call.
Consider replacing this with a standard DeprecationWarning at the top of the module. For example:
# at the top of avocado/utils/script.py
import warnings
warnings.warn(
"The 'script' utility is deprecated. It has been moved to the AAutils "
"project and will be removed after the next LTS release. For more "
"information, please see: https://github.com/avocado-framework/aautils",
DeprecationWarning,
stacklevel=2
)If the custom log_deprecation is required for project-specific logging aggregation, please disregard this suggestion.
References
- PEP 8 recommends that imports should be at the top of the file. The current implementation places the import at the end of the file, which is a deviation from this standard. (link)
| # pylint: disable=wrong-import-position | ||
| from avocado.utils.deprecation import log_deprecation | ||
|
|
||
| log_deprecation.warning("wait") |
There was a problem hiding this comment.
Using a custom log_deprecation object with a bottom-of-file import for deprecation warnings is a possible pattern, but it deviates from standard Python practices. The built-in warnings module is the idiomatic way to handle deprecations.
Using warnings.warn() would:
- Adhere to PEP 8 by allowing the import to be at the top of the file.
- Provide users with standard mechanisms to filter or handle these warnings (e.g., via
warnings.simplefilteror the-Wcommand-line option). - Avoid the need for a custom implementation and a
flush()call.
Consider replacing this with a standard DeprecationWarning at the top of the module. For example:
# at the top of avocado/utils/wait.py
import warnings
warnings.warn(
"The 'wait' utility is deprecated. It has been moved to the AAutils "
"project and will be removed after the next LTS release. For more "
"information, please see: https://github.com/avocado-framework/aautils",
DeprecationWarning,
stacklevel=2
)If the custom log_deprecation is required for project-specific logging aggregation, please disregard this suggestion.
References
- PEP 8 recommends that imports should be at the top of the file. The current implementation places the import at the end of the file, which is a deviation from this standard. (link)
harvey0100
force-pushed
the
deprecation-notices
branch
from
5bf4fb3 to
38acba9
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6256 +/- ##
=======================================
Coverage 73.58% 73.59%
=======================================
Files 206 206
Lines 22497 22509 +12
=======================================
+ Hits 16555 16566 +11
- Misses 5942 5943 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Updated list of announcements in PR's related to migrated modules. Assisted-By: Cursor-Claude-4-Sonnet Signed-off-by: Harvey Lynden <hlynden@redhat.com>
harvey0100
force-pushed
the
deprecation-notices
branch
from
38acba9 to
c15c92c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @avocado/utils/gdb.py:
- Around line 899-902: The current call to log_deprecation.warning("gdb") in
avocado/utils/gdb.py only records the deprecation internally and defers emission
until log_deprecation.flush() is called elsewhere, so direct imports never show
a warning; change this to immediately emit a user-visible deprecation (e.g.,
call warnings.warn(..., DeprecationWarning) or logging.warning(...) with a clear
message and include the AAutils migration guidance), and keep or update
log_deprecation.warning("gdb") if you still want the centralized tracking but
ensure you also call log_deprecation.flush() or directly emit the warning at
import time so it’s not deferred; then add unit tests that import
avocado.utils.gdb directly and assert that a DeprecationWarning or the logging
output is produced when imported.
♻️ Duplicate comments (1)
avocado/utils/ar.py (1)
124-127: Verify that deprecation warnings are visible to users.A previous review suggested using the standard
warningsmodule. While the customlog_deprecationapproach may be intentional for this project, please confirm that users actually see deprecation warnings when importingavocado.utils.ar.Based on the code snippet showing
log_deprecation.warning()implementation, it's unclear whether a visible warning message is emitted to users or if it only maintains internal state. Users should receive clear guidance about the module's deprecated status and migration path.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/autils_migration_announcement.ymlavocado/utils/ar.pyavocado/utils/astring.pyavocado/utils/gdb.pyavocado/utils/process.pyavocado/utils/script.pyavocado/utils/wait.py
🚧 Files skipped from review as they are similar to previous changes (5)
- avocado/utils/wait.py
- avocado/utils/script.py
- avocado/utils/astring.py
- avocado/utils/process.py
- .github/workflows/autils_migration_announcement.yml
🧰 Additional context used
🧬 Code graph analysis (2)
avocado/utils/gdb.py (1)
avocado/utils/deprecation.py (1)
warning(18-25)
avocado/utils/ar.py (1)
avocado/utils/deprecation.py (1)
warning(18-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (42)
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: Linux with Python 3.9
- GitHub Check: Linux with Python 3.13
- GitHub Check: Linux with Python 3.10
- GitHub Check: Linux with Python 3.11
- GitHub Check: Linux with Python 3.12
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: Egg task debian:12.4
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Egg task ubi:8.8
- GitHub Check: Version task ubi:8.8
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Version task debian:12.4
- GitHub Check: Egg task fedora:40
- GitHub Check: Fedora selftests
- GitHub Check: Windows with Python 3.11
- GitHub Check: Build Package (wheel/tarball) for Python 3.10
- GitHub Check: macOS with Python 3.11
- GitHub Check: Static checks
- GitHub Check: Code Coverage (3.11)
| # pylint: disable=wrong-import-position | ||
| from avocado.utils.deprecation import log_deprecation | ||
|
|
||
| log_deprecation.warning("gdb") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the deprecation warning implementation and check if warnings are actually emitted
echo "=== Checking log_deprecation implementation ==="
if [ -f avocado/utils/deprecation.py ]; then
cat avocado/utils/deprecation.py
else
echo "File not found: avocado/utils/deprecation.py"
fi
echo -e "\n=== Searching for log_deprecation usage patterns ==="
rg -n "log_deprecation" --type=py -A 3 -B 1 | head -100
echo -e "\n=== Searching for deprecation warnings module imports ==="
rg -n "import warnings" --type=py | head -20Repository: avocado-framework/avocado
Length of output: 6415
🏁 Script executed:
#!/bin/bash
echo "=== Searching for all flush() calls ==="
rg -n "\.flush\(\)" --type=py -B 2 -A 2
echo -e "\n=== Checking if there are tests for deprecation warnings ==="
fd -t f -n "*test*deprecation*" --type f
rg -n "log_deprecation\|LogDeprecation" -t py testRepository: avocado-framework/avocado
Length of output: 8482
🏁 Script executed:
#!/bin/bash
echo "=== Finding all log_deprecation.flush() calls ==="
rg "log_deprecation\.flush" --type=py -B 5 -A 5
echo -e "\n=== Checking where/when log_deprecation warnings would be emitted ==="
rg "log_deprecation" --type=py | grep -E "(flush|warning)" | head -40
echo -e "\n=== Checking if there are integration tests that verify deprecation warnings ==="
fd test -type f | xargs rg "deprecation" -l 2>/dev/null | head -10Repository: avocado-framework/avocado
Length of output: 2610
🏁 Script executed:
#!/bin/bash
echo "=== Checking for tests related to deprecation ==="
find . -type f -name "*.py" -path "*test*" | xargs rg "log_deprecation\|LogDeprecation" 2>/dev/null
echo -e "\n=== Checking when flush would be called during test execution ==="
rg "flush" avocado/plugins/runners/avocado_instrumented.py -B 10 -A 10
echo -e "\n=== Checking if any CLI or main entry points call flush ==="
rg "log_deprecation" avocado/core --type=py
rg "log_deprecation" avocado/plugins --type=py
echo -e "\n=== Checking documentation or release notes ==="
grep -r "log_deprecation\|LogDeprecation" docs/ 2>/dev/null || echo "No documentation found"Repository: avocado-framework/avocado
Length of output: 1227
🏁 Script executed:
#!/bin/bash
echo "=== Checking logging configuration and where LOGGER output goes ==="
rg "LOGGER\s*=\s*logging.getLogger" avocado/utils/deprecation.py -A 5
echo -e "\n=== Checking if LOGGER.info is sufficient for warnings or if it should be LOGGER.warning ==="
rg "LOGGER\.(warning|warn)" avocado/utils/deprecation.py
echo -e "\n=== Checking logging setup in test execution ==="
rg "logging.basicConfig\|StreamHandler" avocado/core --type=py | head -20
echo -e "\n=== Verifying what happens when importing a deprecated utility outside of tests ==="
echo "The deprecation warning is only added to an internal set when imported."
echo "It is only logged when flush() is called, which happens during test execution."
echo "This means standalone imports of deprecated utilities produce no user-visible warning."Repository: avocado-framework/avocado
Length of output: 728
Critical issue: Deprecation warnings are not shown for direct imports—only during test execution.
The log_deprecation.warning() call at module import only adds the utility name to an internal set and does not emit any user-visible warning. Warnings are deferred and only logged when log_deprecation.flush() is called, which currently happens only once in the codebase during test execution in avocado/plugins/runners/avocado_instrumented.py.
This means users who import deprecated utilities directly (not as part of a test) receive no warning whatsoever, defeating the purpose of deprecation notices. The implementation requires:
- Immediate warning on import (either via
logging.warning()or Python'swarningsmodule) - Ensure the warning is always emitted, not deferred until test execution
- Add tests that verify deprecation warnings are visible to users
- The migration guidance (AAutils reference) is correct but unreachable for non-test usage
🤖 Prompt for AI Agents
In @avocado/utils/gdb.py around lines 899-902, The current call to
log_deprecation.warning("gdb") in avocado/utils/gdb.py only records the
deprecation internally and defers emission until log_deprecation.flush() is
called elsewhere, so direct imports never show a warning; change this to
immediately emit a user-visible deprecation (e.g., call warnings.warn(...,
DeprecationWarning) or logging.warning(...) with a clear message and include the
AAutils migration guidance), and keep or update log_deprecation.warning("gdb")
if you still want the centralized tracking but ensure you also call
log_deprecation.flush() or directly emit the warning at import time so it’s not
deferred; then add unit tests that import avocado.utils.gdb directly and assert
that a DeprecationWarning or the logging output is produced when imported.
1 participant
Updated list of announcements in PR's related
to migrated modules.
Assisted-By: Cursor-Claude-4-Sonnet
Summary by CodeRabbit
Deprecations
Chores
✏️ Tip: You can customize this high-level summary in your review settings.