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

Fix log filter level check. #2663

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

Omar007
Copy link
Contributor

@Omar007 Omar007 commented Sep 16, 2024

Correct errors in the logging changes in #2593.

@Omar007
Copy link
Contributor Author

Omar007 commented Sep 16, 2024

As per #2593 (comment), it seems logging paths are potentially not hit in automated tests. There does seem to be a workflow that starts the application but that did not catch my commit/push mistake.
I tried to add something to the tests directory to check the UnwantedWaitressMessageFilter but I have no idea if these get run or not 🤷

@Omar007 Omar007 force-pushed the development branch 2 times, most recently from e27fcbd to bebc2ff Compare September 16, 2024 09:58
@Omar007
Copy link
Contributor Author

Omar007 commented Sep 16, 2024

Yea doesn't look like those get executed. It should've had a fail on the import for the class, which it did not...

@morpheus65535 morpheus65535 merged commit cc7a800 into morpheus65535:development Sep 16, 2024
2 checks passed
@Omar007 Omar007 deleted the development branch September 16, 2024 16:17
@morpheus65535
Copy link
Owner

@Omar007 Still something wrong here? Using Python 3.8.

Traceback (most recent call last):
  File "/opt/homebrew/opt/[email protected]/Frameworks/Python.framework/Versions/3.8/lib/python3.8/threading.py", line 932, in _bootstrap_inner
    self.run()
  File "/opt/homebrew/opt/[email protected]/Frameworks/Python.framework/Versions/3.8/lib/python3.8/threading.py", line 870, in run
    self._target(*self._args, **self._kwargs)
  File "/Users/morpheus/Desktop/bazarr/bazarr/../libs/waitress/task.py", line 86, in handler_thread
    self.logger.exception("Exception when servicing %r", task)
  File "/opt/homebrew/opt/[email protected]/Frameworks/Python.framework/Versions/3.8/lib/python3.8/logging/__init__.py", line 1481, in exception
    self.error(msg, *args, exc_info=exc_info, **kwargs)
  File "/opt/homebrew/opt/[email protected]/Frameworks/Python.framework/Versions/3.8/lib/python3.8/logging/__init__.py", line 1475, in error
    self._log(ERROR, msg, args, **kwargs)
  File "/opt/homebrew/opt/[email protected]/Frameworks/Python.framework/Versions/3.8/lib/python3.8/logging/__init__.py", line 1589, in _log
    self.handle(record)
  File "/opt/homebrew/opt/[email protected]/Frameworks/Python.framework/Versions/3.8/lib/python3.8/logging/__init__.py", line 1598, in handle
    if (not self.disabled) and self.filter(record):
  File "/opt/homebrew/opt/[email protected]/Frameworks/Python.framework/Versions/3.8/lib/python3.8/logging/__init__.py", line 811, in filter
    result = f.filter(record)
  File "/Users/morpheus/Desktop/bazarr/bazarr/app/logger.py", line 65, in filter
    if record.level < logging.ERROR:
AttributeError: 'LogRecord' object has no attribute 'level'

@morpheus65535
Copy link
Owner

Same thing with 3.11:

Traceback (most recent call last):
  File "/usr/local/Cellar/[email protected]/3.11.7_1/Frameworks/Python.framework/Versions/3.11/lib/python3.11/threading.py", line 1045, in _bootstrap_inner
    self.run()
  File "/usr/local/Cellar/[email protected]/3.11.7_1/Frameworks/Python.framework/Versions/3.11/lib/python3.11/threading.py", line 982, in run
    self._target(*self._args, **self._kwargs)
  File "/Users/morpheus/Desktop/bazarr/bazarr/../libs/waitress/task.py", line 86, in handler_thread
    self.logger.exception("Exception when servicing %r", task)
  File "/usr/local/Cellar/[email protected]/3.11.7_1/Frameworks/Python.framework/Versions/3.11/lib/python3.11/logging/__init__.py", line 1524, in exception
    self.error(msg, *args, exc_info=exc_info, **kwargs)
  File "/usr/local/Cellar/[email protected]/3.11.7_1/Frameworks/Python.framework/Versions/3.11/lib/python3.11/logging/__init__.py", line 1518, in error
    self._log(ERROR, msg, args, **kwargs)
  File "/usr/local/Cellar/[email protected]/3.11.7_1/Frameworks/Python.framework/Versions/3.11/lib/python3.11/logging/__init__.py", line 1634, in _log
    self.handle(record)
  File "/usr/local/Cellar/[email protected]/3.11.7_1/Frameworks/Python.framework/Versions/3.11/lib/python3.11/logging/__init__.py", line 1643, in handle
    if (not self.disabled) and self.filter(record):
                               ^^^^^^^^^^^^^^^^^^^
  File "/usr/local/Cellar/[email protected]/3.11.7_1/Frameworks/Python.framework/Versions/3.11/lib/python3.11/logging/__init__.py", line 830, in filter
    result = f.filter(record)
             ^^^^^^^^^^^^^^^^
  File "/Users/morpheus/Desktop/bazarr/bazarr/app/logger.py", line 65, in filter
    if record.level < logging.ERROR:
       ^^^^^^^^^^^^
AttributeError: 'LogRecord' object has no attribute 'level'. Did you mean: 'levelno'?

@Omar007
Copy link
Contributor Author

Omar007 commented Sep 17, 2024

It should've been fine based on the docs and test file but I guess there's still something else (which also means the test file is setting up the record incorrectly? 😕 ).
Not sure what is going on atm, I'll have to set up a local environment again. I'll see if there is anything I can do about having some kind of test going in GitHub as well but I'm not well-versed in either GitHub's CI nor Python so that'll be fun... At this point I do feel like that is a must though as it looks like there is absolutely nothing in place that actually tests anything, nor any kind of process that verifies changes (2 approvals that say the change was OK?) :'(

EDIT: dove a bit further into Python and looks like the LogRecord constructor splits the level argument into separate fields unlike the args supplied to the makeLogRecord function I had used. I changed the tests over to use the constructor instead and I see the same failure.
I've also played around a bit with the workflows and got something going but that also shows that the tests are in a horrendous state; basically all tests are either failing or broken.
I will put up another MR for this later and would like to request a proper look at it, not just a glance at the syntax. A small heads-up in that regards; there'll need to be some discussion on how to deal with all the broken stuff. Leaving the new test step as is (requiring a full pass or exitcode = 1 and job fails) is probably not an option.

@Omar007 Omar007 mentioned this pull request Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants