scripts: twister: Add TestCase status and reason printing #78345

LukaszMrugala · 2024-09-12T13:25:22Z

When at verbosity 1, we print out the status of TestInstances. However, we omit the TestCases in that print-out. This makes it harder to notice changes at TestCase level, which require perusing the logs.

This PR adds TestCase status and reason printing at verbosity level 2. Reason printing is suppressed if the reason in None or empty.

Visual example:

scripts/pylib/twister/twisterlib/runner.py

nashif · 2024-09-16T11:00:05Z

looks nice, but very busy, wonder if we should have this appear in verbosity level 2(-vv) instead of first (-v) and add a debug flag to replace level 2 now (-vv).

LukaszMrugala · 2024-09-16T14:56:13Z

Incorporated the suggestions. Waiting for the test results.

golowanow · 2024-09-16T16:57:41Z

scripts/pylib/twister/twisterlib/environment.py

-             "verbosity.")
+        help="Call multiple times to increase verbosity.")
+
+    parser.add_argument(


This change should be a separate PR as it affects current output not only for TestCase statuses.

Also something is missing in this change, like this debug output:

zephyr/scripts/pylib/twister/twisterlib/twister_main.py

Lines 139 to 157 in 23eca63

if VERBOSE > 1:

# if we are using command line platform filter, no need to list every

# other platform as excluded, we know that already.

# Show only the discards that apply to the selected platforms on the

# command line

for i in tplan.instances.values():

if i.status == TwisterStatus.FILTER:

if options.platform and i.platform.name not in options.platform:

continue

logger.debug(

"{:<25} {:<50} {}SKIPPED{}: {}".format(

i.platform.name,

i.testsuite.name,

Fore.YELLOW,

Fore.RESET,

i.reason,

)

)

IMO multi -v command line options is more convenient way to set logging levels, whereas -d has so many different meanings including --debug as a special mode of program execution, not just extended log output.

I think that at its core, coupling together debug level and verbosity while not committing to either one is hurting us in the long run.

We use both 'naked' verbosity checks and debug log levels while disallowing specifying them separately. That's not great.

We should either

fully ditch verbosity checks in the code and replace them with custom logging levels enabled by specific verbosity levels

add log level flag in the CLI to enable DEBUG level and still use verbosity to indicate how much information of any level one would like to have.

Given our current approach, the second option is closer to what we currently have. It should probably be in a separate PR.

golowanow · 2024-09-16T17:07:31Z

scripts/pylib/twister/twisterlib/statuses.py

+        match(status):
+            case TwisterStatus.PASS:
+                color = Fore.GREEN
+            case TwisterStatus.SKIP | TwisterStatus.FILTER | TwisterStatus.BLOCK:
+                color = Fore.YELLOW
+            case TwisterStatus.FAIL | TwisterStatus.ERROR:
+                color = Fore.RED
+            case TwisterStatus.STARTED | TwisterStatus.NONE:
+                color = Fore.MAGENTA
+            case _:
+                color = Fore.RESET
+        return color


isn't dictionary lookup simpler for this purpose ?
just thinking of something like

return status2color[status] if status in status2color else Fore.RESET

It would be less concise, as I wouldn't be able to use ORs (|) in the dictionary definition.

It would be less concise, as I wouldn't be able to use ORs (|) in the dictionary definition.

hmm, less concise ? I think this is more pythonic way, and you can even have another palette (grayscale), set it off assigning None, etc.

status2color = { TwisterStatus.PASS: Fore.GREEN, TwisterStatus.SKIP: Fore.YELLOW, TwisterStatus.FILTER: Fore.YELLOW, TwisterStatus.BLOCK: Fore.YELLOW, TwisterStatus.FAIL: Fore.RED, TwisterStatus.ERROR: Fore.RED, TwisterStatus.STARTED: Fore.MAGENTA, TwisterStatus.NONE: Fore.MAGENTA }

besides, no problem, just imo.

Maybe concise wasn't the right word - the Status-Colour relationship is a many-to-one relationship (many statuses can have the same colour), which is immediately apparent in match-case structure. Dictionary is an inherently one-to-one structure (one value for one key). This necessitates repeating colours (YELLOW three times, RED two, MAGENTA two) and it's a bit harder to get information of type "Which Statuses can be yellow?"

and it's a bit harder to get information of type "Which Statuses can be yellow?"

yeah, in case of the match/case construct it is even harder: it needs to be executed for all statuses first to build status2color{} and filter it on values=='yellow'.

When at verbosity 1, we print out the status of TestInstances. This makes it harder to notice changes at TestCase level, which require perusing the logs. This adds TestCase status and reason printing if verbosity level is 2 or more. Reason printing is suppressed if the reason is empty or None. Signed-off-by: Lukasz Mrugala <[email protected]>

gchwier · 2024-09-18T07:41:25Z

scripts/pylib/twister/twisterlib/statuses.py

@@ -19,6 +21,21 @@ def _missing_(cls, value):
        if value is None:
            return TwisterStatus.NONE

+    @staticmethod
+    def get_color(status: TwisterStatus) -> str:
+        match(status):


nit: If you use match keyword, than Twister requires Python 3.10. Therefore, using from __future__ import annotations does not have any sense (it enables some type annotations for lower versions of python)

just mentioned that, because couple days ago @golowanow added PR to still have possibility to run Twister with Python 3.8: #77077
With match keyword it is not possible

I just ran into this on macOS:

❯ scripts/twister --help ZEPHYR_BASE unset, using "/Users/johedber/src/zephyr/zephyr" Traceback (most recent call last): File "/Users/johedber/src/zephyr/zephyr/scripts/twister", line 207, in <module> from twisterlib.twister_main import main File "/Users/johedber/src/zephyr/zephyr/scripts/pylib/twister/twisterlib/twister_main.py", line 16, in <module> from twisterlib.statuses import TwisterStatus File "/Users/johedber/src/zephyr/zephyr/scripts/pylib/twister/twisterlib/statuses.py", line 26 match(status): ^ SyntaxError: invalid syntax ❯ which python3 /usr/bin/python3 ❯ python3 --version Python 3.9.6

nit: If you use match keyword, than Twister requires Python 3.10. Therefore, using from __future__ import annotations does not have any sense (it enables some type annotations for lower versions of python)

You are right - I've used it to change TwisterStatus type hint into Self - it seems I didn't upstream it. My bad.

When it comes to Python<3.10 support - Zephyr does not claim to support it since 3.7.0 LTS (Python 3.10 is listed as minimum requirement), so I do not pay older versions special consideration.

When it comes to Python<3.10 support - Zephyr does not claim to support it since 3.7.0

the problem is confusing user experience to get SyntaxError just for twister --help now.

#78671

LukaszMrugala added Enhancement Changes/Updates/Additions to existing features RFC Request For Comments: want input from the community area: Twister Twister labels Sep 12, 2024

LukaszMrugala force-pushed the lmrugalx_log_testcases branch from 0795f79 to acd4a7a Compare September 13, 2024 11:13

LukaszMrugala marked this pull request as ready for review September 16, 2024 10:00

zephyrbot requested review from gchwier, golowanow, hakehuang, KamilxPaszkiet, nashif and PerMac September 16, 2024 10:01

zephyrbot assigned nashif Sep 16, 2024

golowanow reviewed Sep 16, 2024

View reviewed changes

scripts/pylib/twister/twisterlib/runner.py Outdated Show resolved Hide resolved

scripts/pylib/twister/twisterlib/runner.py Outdated Show resolved Hide resolved

LukaszMrugala force-pushed the lmrugalx_log_testcases branch from acd4a7a to 618d614 Compare September 16, 2024 14:55

golowanow reviewed Sep 16, 2024

View reviewed changes

LukaszMrugala force-pushed the lmrugalx_log_testcases branch from 618d614 to 2b338c5 Compare September 17, 2024 10:21

LukaszMrugala force-pushed the lmrugalx_log_testcases branch from 2b338c5 to 8a30029 Compare September 17, 2024 10:25

golowanow approved these changes Sep 17, 2024

View reviewed changes

hakehuang approved these changes Sep 17, 2024

View reviewed changes

nashif approved these changes Sep 17, 2024

View reviewed changes

nashif merged commit 418b1e0 into zephyrproject-rtos:main Sep 17, 2024
29 checks passed

gchwier reviewed Sep 18, 2024

View reviewed changes

golowanow mentioned this pull request Sep 18, 2024

scripts: twister: Don't use match/case statements #78671

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scripts: twister: Add TestCase status and reason printing #78345

scripts: twister: Add TestCase status and reason printing #78345

LukaszMrugala commented Sep 12, 2024 •

edited

Loading

nashif commented Sep 16, 2024

LukaszMrugala commented Sep 16, 2024

golowanow Sep 16, 2024

LukaszMrugala Sep 17, 2024

golowanow Sep 16, 2024

LukaszMrugala Sep 17, 2024

golowanow Sep 17, 2024 •

edited

Loading

LukaszMrugala Sep 17, 2024

golowanow Sep 17, 2024 •

edited

Loading

gchwier Sep 18, 2024

gchwier Sep 18, 2024

jhedberg Sep 18, 2024

LukaszMrugala Sep 18, 2024

golowanow Sep 18, 2024 •

edited

Loading

	if VERBOSE > 1:
	# if we are using command line platform filter, no need to list every
	# other platform as excluded, we know that already.
	# Show only the discards that apply to the selected platforms on the
	# command line

	for i in tplan.instances.values():
	if i.status == TwisterStatus.FILTER:
	if options.platform and i.platform.name not in options.platform:
	continue
	logger.debug(
	"{:<25} {:<50} {}SKIPPED{}: {}".format(
	i.platform.name,
	i.testsuite.name,
	Fore.YELLOW,
	Fore.RESET,
	i.reason,
	)
	)

scripts: twister: Add TestCase status and reason printing #78345

scripts: twister: Add TestCase status and reason printing #78345

Conversation

LukaszMrugala commented Sep 12, 2024 • edited Loading

nashif commented Sep 16, 2024

LukaszMrugala commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

golowanow Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

golowanow Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

golowanow Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

LukaszMrugala commented Sep 12, 2024 •

edited

Loading

golowanow Sep 17, 2024 •

edited

Loading

golowanow Sep 17, 2024 •

edited

Loading

golowanow Sep 18, 2024 •

edited

Loading