fix: replace bare except Exception with proper error logging#6307
fix: replace bare except Exception with proper error logging#6307MD-Mushfiqur123 wants to merge 1 commit into
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 |
![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 replaces silent exception handling (bare except: pass blocks) with logging statements across several modules, including teststmpdir.py, filelock.py, and genio.py. The review feedback recommends preserving exception details by adding exc_info=True to the logging calls. Additionally, it is suggested to change the log level from WARNING to DEBUG in genio.py to avoid polluting the logs with false-positive warnings when files are expectedly missing.
| LOG.warning("Failed to remove temporary directory '%s'", | ||
| self._dirname) |
There was a problem hiding this comment.
When replacing a bare except: pass with logging, it is highly recommended to preserve the exception details (such as the traceback and error message) by passing exc_info=True. Without this, troubleshooting why the temporary directory could not be removed (e.g., permission issues, directory not empty, etc.) remains difficult.
| LOG.warning("Failed to remove temporary directory '%s'", | |
| self._dirname) | |
| LOG.warning("Failed to remove temporary directory '%s'", | |
| self._dirname, exc_info=True) |
| LOG.debug("Failed to read lock file '%s'", | ||
| self.filename) |
There was a problem hiding this comment.
Similarly, when logging a failure to read the lock file at DEBUG level, including the exception traceback via exc_info=True is extremely helpful for diagnosing lock contention or permission issues during debugging.
| LOG.debug("Failed to read lock file '%s'", | |
| self.filename) | |
| LOG.debug("Failed to read lock file '%s'", | |
| self.filename, exc_info=True) |
| contents = [line.rstrip("\n") for line in file_obj.readlines()] | ||
| except Exception: # pylint: disable=W0703 | ||
| pass | ||
| LOG.warning("Failed to read file '%s'", filename) |
There was a problem hiding this comment.
According to the docstring of read_all_lines, this function is explicitly designed to return an empty list in any error scenario, and is intended to be used when a missing or unreadable file is an expected/acceptable condition. Logging a WARNING here will flood the logs with false-positive warnings during normal operation when files are expectedly missing. Changing this to LOG.debug with exc_info=True avoids log pollution while still preserving the ability to troubleshoot actual issues when debug logging is enabled.
| LOG.warning("Failed to read file '%s'", filename) | |
| LOG.debug("Failed to read file '%s'", filename, exc_info=True) |
|
I see these utils have been migrated to AAutils. Closing this PR. |
github-project-automation
Bot
moved this from Review Requested
to Done 114
in Default project
1 participant
Closing - the affected utils have been migrated to AAutils project.