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: Add quarantine feature #33287
twister: Add quarantine feature #33287
Conversation
To try run:
and check the result reports. |
thanks, will take a look. unrelated to the content of the PR, are you adding the related issue directly on the right sidebar? Can you please instead use the 'Fixes #XYZ' in the PR body? |
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.
just have one enhancement comments, looks good to me
quarantine.yaml
Outdated
list1: | ||
platforms: all | ||
scenarios: | ||
- sample.basic.helloworld |
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.
suggest to add a github issue link here
scripts/pylib/twister/twisterlib.py
Outdated
instance.testcase.id]) | ||
if test_configuration in self.quarantine: | ||
discards[instance] = discards.get(instance, | ||
"Quarantine") |
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.
just wonder whether it is possible to add the github issue link into report, so that we can tarck the latest status
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.
It is a good idea to be able to easily track related bugs. The simplest, for now, is to add comments into quarantine.yaml with the related links and whatever information the user wants. Is the link useful in the report? We can discuss this during the testing wg meeting.
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.
tested and it worked well.
scripts/twister
Outdated
@@ -464,6 +464,15 @@ Artificially long but functional example: | |||
action="store", | |||
help="Load list of tests and platforms to be run from file.") | |||
|
|||
parser.add_argument( | |||
"-q", |
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 guess it's better not to use '-q' for this purpose as usually '-q' stands for 'quiet'. So it's quite misleading in this case.
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 removed -q
, only full --quarantine-list
will work now
summary feedback in weekly meeting:
|
quarantine.yaml
Outdated
@@ -0,0 +1,20 @@ | |||
# list of quarantined tests |
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.
Just a nitpick - probably we can move quarantine.yaml
to tests/
directory so we won't overwhelm zephyr home directory?
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.
This file will be removed from this dir and will probably end in docs as an example. For now, it just serves as a showcase to play with for the review purpose.
quarantine.yaml
Outdated
@@ -0,0 +1,20 @@ | |||
# list of quarantined tests | |||
|
|||
list1: |
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.
maybe move to a list:
- platforms:
- native_posix
scenarios:
- posix.eventfd_basic.posix_api
- platforms:
- qemu_cortex_m3
- native_posix
scenarios:
- kernel.common
- kernel.common.misra
- kernel.common.nano64
and add schema verification as well
quarantine.yaml
Outdated
platforms: | ||
- native_posix | ||
scenarios: | ||
- posix.eventfd_basic.posix_api |
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.
add a details section for documenting why something is being quarantined.
6f579fa
to
cac6e3e
Compare
@nashif @hakehuang @chen-png Can you please check again this PR? I've added the requested changes/features:
I made separate commits for these changes so it is easier to follow what has changed. In the end, I will squash some of them. |
|
cac6e3e
to
3ad3bfe
Compare
if this is ready for review, move it out of draft please. |
a56b603
to
4613ce7
Compare
Adds feature allowing to use yaml file with dictionaries defining tests to be quarantined (extra arg "--quarantine-list FILENAME"). The dictionaries are validated according to the proper schema and loaded. A flat list is created containing quarantined configurations (configuration = platform + scenario). Configurations under quarantine are skipped and get "Quarantine" as a reason in the results reports. A "comment" can be added to a quarantine entry in the quarantine yaml with more details (e.g. issue #) and it will be also added to the report. The status of tests under quarantine can be verify if `--quarantine-verify` is used in addition to "--quarantine-list FILENAME". Using these args will make twister skip all tests which are not on the quarantine list. Signed-off-by: Maciej Perkowski <Maciej.Perkowski@nordicsemi.no>
4613ce7
to
8a4606d
Compare
@nashif ready for review |
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.
minor, otherwise lgtm
doc/guides/test/twister.rst
Outdated
@@ -634,3 +634,39 @@ using an external J-Link probe. The "probe_id" keyword overrides the | |||
product: DAPLink CMSIS-DAP | |||
runner: jlink | |||
serial: null | |||
|
|||
Quarantine | |||
+++++++++++++++++++++++++++ |
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.
make this match length of title and add space below.
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.
fixed
Add section about quarantine feature Signed-off-by: Maciej Perkowski <Maciej.Perkowski@nordicsemi.no>
5268522
to
9197130
Compare
twister: Add quarantine feature
Adds feature allowing to use yaml file with dictionaries defining
tests to be quarantined (extra arg "--quarantine-list FILENAME").
The dictionaries are validated according to the proper schema
and loaded.
A flat list is created containing quarantined configurations
(configuration = platform + scenario). Configurations under quarantine
are skipped and get "Quarantine" as a reason in the results reports.
A "comment" can be added to a quarantine entry in the quarantine yaml
with more details (e.g. issue #) and it will be also added to
the report.
The status of tests under quarantine can be verify if
--quarantine-verify
is used in addition to"--quarantine-list FILENAME". Using these args will make twister skip
all tests which are not on the quarantine list.
Fixes #33207