-
Notifications
You must be signed in to change notification settings - Fork 18
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
trunner: provide traceback for internal exceptions when waiting for plo #272
base: master
Are you sure you want to change the base?
Conversation
IMHO this should be solved in other way (maybe change how phoenixd exception is printed depending on verbosity in raw logs / test summary. Wrapping all phoenixd errors as Please note that the original exception traceback is not lost, just not explicitly printed. |
4508d48
to
a62c125
Compare
8401865
to
1c7e694
Compare
trunner/tools/phoenix.py
Outdated
@@ -24,7 +25,7 @@ def wait_for_vid_pid(vid: int, pid: int, timeout=0): | |||
found_ports = [port for port in list_ports.comports() if port.pid == pid and port.vid == vid] | |||
|
|||
if len(found_ports) > 1: | |||
raise Exception( | |||
raise AssertionError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO this is not an Assertion error (or convert it to assert), maybe ValueError
would be more in place here?
trunner/tools/phoenix.py
Outdated
except (TimeoutError, AssertionError) as exc: | ||
raise PhoenixdError(msg=str(exc)) from exc | ||
except Exception as exc: | ||
raise PhoenixdError(msg=str(exc), output=traceback.format_exc()) from exc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, it looks strange that the traceback is under OUTPUT
, imho would be better to format the PhoenixdError
exception directly inside the the exception implementation, sample code below (not tested):
class ProcessError(HarnessError):
name: str = "PROCESS"
# [...]
def __str__(self) -> str:
# TODO format github actions output
err = [bold(f"{self.name} ERROR: ") + self.msg]
if self.output is not None:
err.extend([bold("OUTPUT:"), self.output])
err.extend(self._format_additional_info())
# ADDED CODE:
# add traceback if it's rather an implementation error instead of a logic one
if not isinstance(self.__cause__, TimeoutError):
err.append(bold("TRACEBACK:"))
err.extend(traceback.format_exception(self))
err.append("")
return "\n".join(err)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looked through all ProcessErrors used in test runner and in most of the cases traceback seems to be unnecessary (the line in test runner is then shown), I'd like to print it only for internal exceptions, where we don't know the exact line. That's why I've proposed (last fp) adding it only when Exception
is raised and labeled as TRACEBACK
:
What do you think?
3c5be3b
to
aed2461
Compare
aed2461
to
de0e94b
Compare
raise PhoenixdError(str(exc)) from exc | ||
except Exception as exc: | ||
# add traceback if it's an internal exception | ||
raise PhoenixdError(str(exc), traceback=traceback.format_exc()) from exc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replacing exception traceback is kindof misleading (no PhoenixdErorr exception is raised at the line provided in traceback). My proposed solution provided real traceback (2 tracebacks because of from
usage)
Description
Before:
After:
The mentioned issue has been described on Confluence in Github Known Issues - point 2.5.
Motivation and Context
Provide more information about internal serial exceptions.
Types of changes
How Has This Been Tested?
Checklist:
Special treatment