trunner: provide traceback for internal exceptions when waiting for plo

[damianloew]

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

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

How Has This Been Tested?

• Already covered by automatic testing.
• New test added: (add PR link here).
• Tested by hand on: (list targets here).

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing linter checks and tests passed.
• My changes generate no new compilation warnings for any of the targets.

Special treatment

• This PR needs additional PRs to work (list the PRs, preferably in merge-order).
• I will merge this PR by myself when appropriate.

[github-actions]

Unit Test Results

5 949 tests  ±0   5 296 ✔️ ±0   30m 49s ⏱️ - 1m 43s
   325 suites ±0      653 💤 ±0 
       1 files   ±0          0 ❌ ±0 

Results for commit de0e94b. ± Comparison against base commit a37c915.

♻️ This comment has been updated with latest results.

[nalajcie]

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 PhoenixException is a nice way of abstracting things and makes it easier to use.

Please note that the original exception traceback is not lost, just not explicitly printed.

[damianloew]

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 PhoenixException is a nice way of abstracting things and makes it easier to use.

Please note that the original exception traceback is not lost, just not explicitly printed.

Did you mean sth like the changes in the last force push?
To leave the phoenix exceptions simple I'd print traceback as output:

[nalajcie]

IMHO this is not an Assertion error (or convert it to assert), maybe ValueError would be more in place here?

[nalajcie]

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)

[damianloew]

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?

[nalajcie]

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)