Skip to content

twister: Adjust status for quarantined instances #90617

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.

Sign up for GitHub

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

Merged
kartben merged 2 commits into from
Jun 5, 2025
Merged

twister: Adjust status for quarantined instances #90617

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
272eae3
twister: logging: decouple status reporting from logging level
PerMac May 30, 2025
87ab939
twister: Adjust status for quarantined instances
PerMac May 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion scripts/pylib/twister/twisterlib/reports.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,7 +734,7 @@ def summary(self, results, duration):
f'.'
)

built_only = results.total - run - results.filtered_configs
built_only = results.total - run - results.filtered_configs - results.skipped
logger.info(
f"{Fore.GREEN}{run}{Fore.RESET} test configurations executed on platforms,"
f" {TwisterStatus.get_color(TwisterStatus.NOTRUN)}{built_only}{Fore.RESET}"
Expand Down
6 changes: 5 additions & 1 deletion scripts/pylib/twister/twisterlib/runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1885,7 +1885,7 @@ def run(self):
self.results.done -= self.results.error
self.results.error = 0
else:
self.results.done = self.results.filtered_static
self.results.done = self.results.filtered_static + self.results.skipped

self.execute(pipeline, done_queue)

Expand Down Expand Up @@ -1923,6 +1923,10 @@ def update_counting_before_pipeline(self):
self.results.filtered_configs_increment()
self.results.filtered_cases_increment(len(instance.testsuite.testcases))
self.results.cases_increment(len(instance.testsuite.testcases))
elif instance.status == TwisterStatus.SKIP and "overflow" not in instance.reason:
self.results.skipped_increment()
self.results.skipped_cases_increment(len(instance.testsuite.testcases))
self.results.cases_increment(len(instance.testsuite.testcases))
elif instance.status == TwisterStatus.ERROR:
self.results.error_increment()

Expand Down
6 changes: 4 additions & 2 deletions scripts/pylib/twister/twisterlib/testplan.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,10 +605,12 @@ def handle_quarantined_tests(self, instance: TestInstance, plat: Platform):
sim_name
)
if matched_quarantine and not self.options.quarantine_verify:
instance.add_filter("Quarantine: " + matched_quarantine, Filters.QUARANTINE)
instance.status = TwisterStatus.SKIP
instance.reason = "Quarantine: " + matched_quarantine
return
if not matched_quarantine and self.options.quarantine_verify:
instance.add_filter("Not under quarantine", Filters.QUARANTINE)
instance.status = TwisterStatus.SKIP
instance.reason = "Not under quarantine"

def load_from_file(self, file, filter_platform=None):
if filter_platform is None:
Expand Down
24 changes: 15 additions & 9 deletions scripts/pylib/twister/twisterlib/twister_main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,20 +104,26 @@ def twister(options: argparse.Namespace, default_options: argparse.Namespace):
logger.error(f"{e}")
return 1

if options.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
# 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

if options.verbose > 0:
for i in tplan.instances.values():
if i.status == TwisterStatus.FILTER:
if i.status in [TwisterStatus.SKIP,TwisterStatus.FILTER]:
if options.platform and not tplan.check_platform(i.platform, options.platform):
continue
logger.debug(
# Filtered tests should be visable only when verbosity > 1
if options.verbose < 2 and i.status == TwisterStatus.FILTER:
continue
res = i.reason
if "Quarantine" in i.reason:
res = "Quarantined"
logger.info(
Copy link
Member

@nashif nashif Jun 3, 2025

Choose a reason for hiding this comment

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

This is very verbose, even in debug mode, given that the comment field is pretty much freestyle and someone might end up writing a novel there. Maybe we should have a field for an issue number, so it is managed and can be added to the console output, right now:

image

Copy link
Member Author

@PerMac PerMac Jun 3, 2025

Choose a reason for hiding this comment

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

If I understand the issue is the length of messages, and not the change in logger level, right?
I am worried that unify field for a number might not work, since quarantine is not only for upstream zephyr. We use it also linking to internal tickets, and I assume some users might not even care that much about such linking in their projects.
What about some simplified generic comment: "Quarantined, check reports for details", or even just "Quarantined"?

Copy link
Member

@nashif nashif Jun 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If I understand the issue is the length of messages, and not the change in logger level, right?

yes, putting a freestyle comment in the output here is asking for trouble. A comment might be anything.

I am worried that unify field for a number might not work, since quarantine is not only for upstream zephyr.

Then do not use it downstream, or use your own numbering scheme or whatever, not using it downstream is not a good reason for not adding features and usabiliy upstream.

We use it also linking to internal tickets, and I assume some users might not even care that much about such linking in their projects. What about some simplified generic comment: "Quarantined, check reports for details", or even just "Quarantined"?

This is not linking anything, this is a reference only, if an issue field exists, it will be shown as:

Quarantied: see #<issue number>

instead of a full link based on a comment.

issue number can be anything and you can use your own numbering/reference scheme downstream.

For now, you can just leave it as "Quarantined", that reference field thing can be done later.

Copy link
Member Author

@PerMac PerMac Jun 3, 2025

Choose a reason for hiding this comment

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

btw now we already will need some differentiation between "quarantine" skips and "other" skips, (not showing the whole reason in logs). I like the idea of some extra formatting field for issue number, but please keep in mind that it will add even more tricky handling of skip types at reporting level. that's why I preferred to have a dedicated status for quarantined items.

f"{i.platform.name:<25} {i.testsuite.name:<50}"
f" {Fore.YELLOW}FILTERED{Fore.RESET}: {i.reason}"
)
f" {Fore.YELLOW}{i.status.upper()}{Fore.RESET}: {res}"
)

report = Reporting(tplan, env)
plan_file = os.path.join(options.outdir, "testplan.json")
Expand Down
15 changes: 13 additions & 2 deletions scripts/tests/twister/test_runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2545,6 +2545,8 @@ def mock_client_from_environ(jobs):
results_mock().iteration = 0
results_mock().failed = 2
results_mock().total = 9
results_mock().filtered_static = 0
results_mock().skipped = 0

def iteration_increment(value=1, decrement=False):
results_mock().iteration += value * (-1 if decrement else 1)
Expand Down Expand Up @@ -2608,7 +2610,7 @@ def test_twisterrunner_update_counting_before_pipeline():
),
'dummy5': mock.Mock(
status=TwisterStatus.SKIP,
reason=None,
reason="Quarantine",
testsuite=mock.Mock(
testcases=[mock.Mock()]
)
Expand All @@ -2629,6 +2631,7 @@ def test_twisterrunner_update_counting_before_pipeline():
error = 0,
cases = 0,
filtered_cases = 0,
skipped = 0,
skipped_cases = 0,
failed_cases = 0,
error_cases = 0,
Expand All @@ -2652,14 +2655,22 @@ def cases_increment(value=1, decrement=False):
def filtered_cases_increment(value=1, decrement=False):
tr.results.filtered_cases += value * (-1 if decrement else 1)
tr.results.filtered_cases_increment = filtered_cases_increment
def skipped_increment(value=1, decrement=False):
tr.results.skipped += value * (-1 if decrement else 1)
tr.results.skipped_increment = skipped_increment
def skipped_cases_increment(value=1, decrement=False):
tr.results.skipped_cases += value * (-1 if decrement else 1)
tr.results.skipped_cases_increment = skipped_cases_increment

tr.update_counting_before_pipeline()

assert tr.results.filtered_static == 1
assert tr.results.filtered_configs == 1
assert tr.results.filtered_cases == 4
assert tr.results.cases == 4
assert tr.results.cases == 5
assert tr.results.error == 1
assert tr.results.skipped == 1
assert tr.results.skipped_cases == 1


def test_twisterrunner_show_brief(caplog):
Expand Down
4 changes: 2 additions & 2 deletions scripts/tests/twister/test_testplan.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,11 +342,11 @@ def test_quarantine_short(class_testplan, platforms_list, test_data,
if testname in expected_val:
assert instance.status == TwisterStatus.NONE
else:
assert instance.status == TwisterStatus.FILTER
assert instance.status == TwisterStatus.SKIP
assert instance.reason == "Not under quarantine"
else:
if testname in expected_val:
assert instance.status == TwisterStatus.FILTER
assert instance.status == TwisterStatus.SKIP
assert instance.reason == "Quarantine: " + expected_val[testname]
else:
assert instance.status == TwisterStatus.NONE
Expand Down
22 changes: 14 additions & 8 deletions scripts/tests/twister_blackbox/test_quarantine.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import json

# pylint: disable=duplicate-code
# pylint: disable=no-name-in-module
from conftest import ZEPHYR_BASE, TEST_DATA, testsuite_filename_mock
from twisterlib.testplan import TestPlan

Expand Down Expand Up @@ -49,10 +50,15 @@ def test_quarantine_verify(self, out_path):

with open(os.path.join(out_path, 'testplan.json')) as f:
j = json.load(f)

# Quarantine-verify "swaps" statuses. The ones that are in quarantine list
# should no longer be quarantined, and the ones that are not in the list
# should be quarantined. Remove "quarantined" tests from "verify" testplan
# to count what should be verified.
filtered_j = [
(ts['platform'], ts['name'], tc['identifier']) \
(ts['platform'], ts['name']) \
for ts in j['testsuites'] \
for tc in ts['testcases'] if 'reason' not in tc
if ts['status'] != "skipped"
]

assert str(sys_exit.value) == '0'
Expand Down Expand Up @@ -89,26 +95,26 @@ def test_quarantine_list(self, capfd, out_path, test_path, test_platforms, quara
sys.stdout.write(out)
sys.stderr.write(err)

board1_match1 = re.search('agnostic/group2/dummy.agnostic.group2 FILTERED: Quarantine: test '
board1_match1 = re.search('agnostic/group2/dummy.agnostic.group2 SKIPPED: Quarantine: test '
'intel_adl_crb', err)
board1_match2 = re.search(
'agnostic/group1/subgroup2/dummy.agnostic.group1.subgroup2 FILTERED: Quarantine: test '
'agnostic/group1/subgroup2/dummy.agnostic.group1.subgroup2 SKIPPED: Quarantine: test '
'intel_adl_crb',
err)
qemu_64_match = re.search(
'agnostic/group1/subgroup2/dummy.agnostic.group1.subgroup2 FILTERED: Quarantine: test '
'agnostic/group1/subgroup2/dummy.agnostic.group1.subgroup2 SKIPPED: Quarantine: test '
'qemu_x86_64',
err)
all_platforms_match = re.search(
'agnostic/group1/subgroup1/dummy.agnostic.group1.subgroup1 FILTERED: Quarantine: test '
'agnostic/group1/subgroup1/dummy.agnostic.group1.subgroup1 SKIPPED: Quarantine: test '
'all platforms',
err)
all_platforms_match2 = re.search(
'agnostic/group1/subgroup1/dummy.agnostic.group1.subgroup1 FILTERED: Quarantine: test '
'agnostic/group1/subgroup1/dummy.agnostic.group1.subgroup1 SKIPPED: Quarantine: test '
'all platforms',
err)
all_platforms_match3 = re.search(
'agnostic/group1/subgroup1/dummy.agnostic.group1.subgroup1 FILTERED: Quarantine: test '
'agnostic/group1/subgroup1/dummy.agnostic.group1.subgroup1 SKIPPED: Quarantine: test '
'all platforms',
err)

Expand Down
Loading