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

twister: harness: Fix several issues at Console harness #63902

Conversation

golowanow
Copy link
Member

@golowanow golowanow commented Oct 13, 2023

Twister Console Harness several fixes and small improvements:

  • Fix unordered pattern matching to treat the ztest as failed when not all of the expected patterns were found in the console output, but the ztest application itself reports 'PROJECT EXECUTION SUCCESSFUL'.
    Extends twister: harness: Fix Console pattern matching for ztest #63621

  • Unify debug logging on pattern match for ordered and unordered patterns.

  • Check for non-empty pattern list configured for Console: If Console Harness has no regexp pattens configured, then ConfigurationError is raised, handled, and the failure of the test case correctly accounted in the summary results.

  • Implement a workaround for Console harness to compose TestCase identifier correctly when Ztest suite with a single TestCase is executed (generally, Ztest suite should use Ztest harness).
    Without the fix each TestCase result at Console is duplicated (and written into twister.json) with short 'identifier' set to TestSuite id only, without TestCase suffix added. The resulting entry with the full TestCase id is also stored, but its values are set empty to defaults.

@golowanow golowanow marked this pull request as ready for review October 13, 2023 07:27
@golowanow golowanow requested a review from nashif as a code owner October 13, 2023 07:27
@zephyrbot zephyrbot added the area: Twister Twister label Oct 13, 2023
@golowanow golowanow added the bug The issue is a bug, or the PR is fixing a bug label Oct 13, 2023
@golowanow golowanow force-pushed the twister_fix_console_patterns_unordered branch from 73984f3 to 3dd7717 Compare October 13, 2023 19:20
@golowanow golowanow changed the title twister: harness: Fix Console unordered pattern matching for ztest twister: harness: Fix several issues at Console harness Oct 13, 2023
@golowanow golowanow requested a review from gchwier October 13, 2023 19:29
gchwier
gchwier previously approved these changes Oct 16, 2023
Copy link
Collaborator

@gchwier gchwier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, one minor comment

scripts/pylib/twister/twisterlib/harness.py Outdated Show resolved Hide resolved
@golowanow golowanow force-pushed the twister_fix_console_patterns_unordered branch from 3dd7717 to 9edae34 Compare October 17, 2023 06:18
@golowanow golowanow requested a review from gchwier October 17, 2023 06:19
gchwier
gchwier previously approved these changes Oct 17, 2023
@golowanow
Copy link
Member Author

@nashif, @galak, @gopiotr, @hakehuang, @PerMac - up to your attention

PerMac
PerMac previously approved these changes Oct 23, 2023
@nashif
Copy link
Member

nashif commented Oct 23, 2023

  • Implement a workaround for Console harness to compose TestCase identifier correctly when only one TestCase is recognized as executed.
    Without the fix each TestCase result at Console was duplicated (and written into twister.json) with short 'identifier' set to TestSuite id only, without TestCase suffix added.
    The resulting entry with the full TestCase id was also stored, but without its values set being empty with defaults.

can you point to an example where this happens?

@golowanow
Copy link
Member Author

  • Implement a workaround for Console harness to compose TestCase identifier correctly when only one TestCase is recognized as executed.
    Without the fix each TestCase result at Console was duplicated (and written into twister.json) with short 'identifier' set to TestSuite id only, without TestCase suffix added.
    The resulting entry with the full TestCase id was also stored, but without its values set being empty with defaults.

can you point to an example where this happens?

current main branch (92045a02) with ./scripts/twister -vv -j 1 -p qemu_x86 -T tests/subsys/debug/coredump_backends

{
    "environment":{
        "os":"posix",
        "zephyr_version":"zephyr-v3.5.0-324-g92045a021ed3",
        "toolchain":"zephyr",
        "commit_date":"2023-10-23T14:32:39+02:00",
        "run_date":"2023-10-23T13:07:05+00:00"
    },
    "testsuites":[
        {
            "name":"tests/subsys/debug/coredump_backends/debug.coredump.backends.logging",
            "arch":"x86",
            "platform":"qemu_x86",
            "path":"tests/subsys/debug/coredump_backends",
            "run_id":"021ad5ff8f59fbf1f913df452da28c22",
            "runnable":true,
            "retries":0,
            "status":"passed",
            "execution_time":"2.15",
            "testcases":[
                {
                    "identifier":"debug.coredump.backends.logging.coredump_backend",
                    "execution_time":"0.00",
                    "status":null
                },
                {
                    "identifier":"debug.coredump.backends.logging",
                    "execution_time":"0.00",
                    "status":"passed"
                }
            ]
        },

with this fix:

{
    "environment":{
        "os":"posix",
        "zephyr_version":"v3.5.0-rc2-203-g9edae344d00d",
        "toolchain":"zephyr",
        "commit_date":"2023-10-17T08:17:24+02:00",
        "run_date":"2023-10-23T13:12:07+00:00"
    },
    "testsuites":[
        {
            "name":"tests/subsys/debug/coredump_backends/debug.coredump.backends.logging",
            "arch":"x86",
            "platform":"qemu_x86",
            "path":"tests/subsys/debug/coredump_backends",
            "run_id":"533f73cf18c87813a3aaf17ec40faef3",
            "runnable":true,
            "retries":0,
            "status":"passed",
            "execution_time":"2.13",
            "testcases":[
                {
                    "identifier":"debug.coredump.backends.logging.coredump_backend",
                    "execution_time":"2.13",
                    "status":"passed"
                }
            ]
        },

@nashif
Copy link
Member

nashif commented Oct 23, 2023

current main branch (92045a02) with ./scripts/twister -vv -j 1 -p qemu_x86 -T tests/subsys/debug/coredump_backends

ok. While your workaround might deal with the issue, the problem here is that the test is wrong. It can't be both a ztest and a console test. We either use ztest and rely on passing tests reporting based on ztest output or we rely on the console output, having both is wrong.

@@ -162,6 +163,14 @@ class Console(Harness):

def configure(self, instance):
super(Console, self).configure(instance)
if self.regex is None or len(self.regex) == 0:
self.state = "failed"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the definition of the testcase is wrong, this should be treated as an error. So, instead of doing this here and calling this a fail, error already during the parsing of the test (in config_parser.py)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if this can be dealt with on the schema level, i.e.. disallowing empty sequences.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if this can be dealt with on the schema level, i.e.. disallowing empty sequences.

pykwalify has allowempty but for `map' type only
https://pykwalify.readthedocs.io/en/unstable/validation-rules.html#allowempty

Copy link
Member Author

@golowanow golowanow Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the definition of the testcase is wrong, this should be treated as an error.

changed to 'error' status for instances

So, instead of doing this here and calling this a fail, error already during the parsing of the test (in config_parser.py)

it seems not easy to avoid these checks here with the current twister harness design: testsute-schema.yaml defines common harness_config for all types of harnesses simultaneously and parser checks for its presence with allowed (not required) properties. Later harness configure() needs to check what it got from the test instance in regard of the specific harness class.

@golowanow golowanow dismissed stale reviews from PerMac and gchwier via c670eed October 31, 2023 11:37
@golowanow golowanow force-pushed the twister_fix_console_patterns_unordered branch from 9edae34 to c670eed Compare October 31, 2023 11:37
@golowanow golowanow requested review from nashif and gchwier October 31, 2023 12:34
@golowanow golowanow requested a review from PerMac October 31, 2023 12:34
@golowanow golowanow force-pushed the twister_fix_console_patterns_unordered branch from c670eed to 37f57b6 Compare October 31, 2023 12:55
Fix the Twister Console harness unordered pattern matching
to treat the ztest as failed when not all of the
expected patterns were found in the console output, but the ztest
application itself reports 'PROJECT EXECUTION SUCCESSFUL'.

Unify debug logging on pattern match for ordered and unordered patterns.

Signed-off-by: Dmitrii Golovanov <[email protected]>
If Console Harness 'harness_config' properties have no 'regex'
patterns or no correct 'type' set, then ConfigurationError exception
is raised, handled, and the test instance error is accounted
in the summary results.

Signed-off-by: Dmitrii Golovanov <[email protected]>
@golowanow golowanow force-pushed the twister_fix_console_patterns_unordered branch from 37f57b6 to 52c39d6 Compare November 2, 2023 11:18
gchwier
gchwier previously approved these changes Nov 3, 2023
@golowanow
Copy link
Member Author

golowanow commented Nov 8, 2023

current main branch (92045a02) with ./scripts/twister -vv -j 1 -p qemu_x86 -T tests/subsys/debug/coredump_backends

ok. While your workaround might deal with the issue, the problem here is that the test is wrong. It can't be both a ztest and a console test. We either use ztest and rely on passing tests reporting based on ztest output or we rely on the console output, having both is wrong.

@nashif , that particular testcase wrong implementation (coredump_backends.logging) is now addressed by #64937

Console Harness id has only TestSuite id without TestCase name suffix.
TODO: Only the first TestCase name is taken if available.
'''
if self.instance is not None and len(self.instance.testcases) == 1:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in python shall we just use below?

Suggested change
if self.instance is not None and len(self.instance.testcases) == 1:
if self.instance and self.instance.testcases:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this workaround is applicable with a single Ztest testcase only.
I've simplified this if statement; also better description is added for this method as well as for the commit message.

else:
self.state = "failed"
tc = self.instance.set_case_status_by_name(
self.id,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use self.get_testcase_name() for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, I've forgot it here.

Implement a workaround for Console harness to compose TestCase
identifier correctly when a Ztest suite with a single testcase
uses this harness type. Normally, a Ztest suite should use the
Ztest Twister harness.
Without this workaround each Ztest TestCase result on Console is
duplicated (and written into twister.json) with its 'identifier'
attribute set to TestSuite id only, no TestCase suffix added;
the resulting entry with the full TestCase id is also stored,
but its values are empty with defaults.

Signed-off-by: Dmitrii Golovanov <[email protected]>
@golowanow golowanow force-pushed the twister_fix_console_patterns_unordered branch from 52c39d6 to 2ea008e Compare November 8, 2023 19:06
@golowanow
Copy link
Member Author

@nashif, @galak, @gchwier, @gopiotr, @hakehuang, @PerMac - up to your attention

@nashif nashif merged commit 7d0d3f8 into zephyrproject-rtos:main Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Twister Twister bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants