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: Add quarantine feature #33287

Merged
merged 2 commits into from Apr 12, 2021

Conversation

PerMac
Copy link
Member

@PerMac PerMac commented Mar 12, 2021

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

@PerMac PerMac requested review from nashif and chrta Mar 12, 2021
@github-actions github-actions bot added the area: Twister Twister label Mar 12, 2021
@PerMac PerMac linked an issue Mar 12, 2021 that may be closed by this pull request
@PerMac PerMac marked this pull request as draft Mar 12, 2021
@PerMac
Copy link
Member Author

PerMac commented Mar 12, 2021

To try run:

scripts/twister -T samples/hello_world/ -T tests/posix/eventfd_basic/ -T tests/kernel/common/ -p qemu_cortex_m3 -p native_posix --platform-reports --quarantine-list quarantine.yaml

and check the result reports.
@andyross @galak @hakehuang @erwango @chen-png I've marked you here as I thought you might be interested in that feature and want to play a bit with it. I would appreciate your input. I believe it can be really useful for on-target CIs and results publishing. It would allow to easily "turn off" the failing tests and got it marked in the results as "Quarantine" so it won't block the publishing due to hitting the failure threshold. It should also improve the execution time, as faulty tests won't be executed and retry-failed won't consume the time.
@nashif It also should help in results visualization in grafana, where only "Quarantine" skips can be counted for pie charts and pass/fail/skip ratios
The proposed implementation leaves some open space for expanding the functionality. I didn't add an option to skip the whole test suite (basically all scenarios in yaml) as there is a twister refactoring going on and it is better to wait till then than implementing some temp workarounds.)
If the proposed solution is accepted I will add the proper documentation and the example of quarantine.yaml will end up there.

@nashif
Copy link
Member

nashif commented Mar 12, 2021

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?

Copy link
Collaborator

@hakehuang hakehuang left a comment

just have one enhancement comments, looks good to me

quarantine.yaml Outdated
list1:
platforms: all
scenarios:
- sample.basic.helloworld
Copy link
Collaborator

@hakehuang hakehuang Mar 15, 2021

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

instance.testcase.id])
if test_configuration in self.quarantine:
discards[instance] = discards.get(instance,
"Quarantine")
Copy link
Collaborator

@hakehuang hakehuang Mar 15, 2021

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

Copy link
Member Author

@PerMac PerMac Mar 15, 2021

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.

Copy link
Collaborator

@chen-png chen-png left a comment

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",
Copy link
Collaborator

@evgeniy-paltsev evgeniy-paltsev Mar 15, 2021

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.

Copy link
Member Author

@PerMac PerMac Mar 25, 2021

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

@hakehuang
Copy link
Collaborator

hakehuang commented Mar 15, 2021

summary feedback in weekly meeting:

  • Need to explain why cases are in quarantine list, maybe add a comment with link to github issue would be a solution.
  • Need to document the quarantine yaml schema.
  • Maybe we can use this quarantine list file as input for a checking test plan. Such as all the excluded cases can be checked whether runnable or not.
  • Quarantine cases may not be reported as skipped, should be a special quarantine message=”” type=”” currently it is
    skipped message=”quarantine” type=”skip”

quarantine.yaml Outdated
@@ -0,0 +1,20 @@
# list of quarantined tests
Copy link
Collaborator

@evgeniy-paltsev evgeniy-paltsev Mar 15, 2021

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?

Copy link
Member Author

@PerMac PerMac Mar 22, 2021

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:
Copy link
Member

@nashif nashif Mar 15, 2021

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
Copy link
Member

@nashif nashif Mar 15, 2021

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.

@PerMac PerMac force-pushed the enchancement/quarantine_list branch 2 times, most recently from 6f579fa to cac6e3e Compare Mar 25, 2021
@PerMac
Copy link
Member Author

PerMac commented Mar 25, 2021

@nashif @hakehuang @chen-png Can you please check again this PR? I've added the requested changes/features:

  • comments can be added in the quarantine list (e.g. link to PR) which will also end up in the report
  • additional arg --quarantine-verify can be used which will make twister filter out everything which is not in the quarantine list (to verify only the statuses of quarantine items)
  • I added a schema verification for the .yaml file

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.
The quarantine.yaml will be removed and will end up in docs as an example of how to use the quarantine.
I will add docs when it is agreed that the proposed solution is fine

@PerMac
Copy link
Member Author

PerMac commented Mar 25, 2021

I realized that in one of the commits I accidentally make pycharm refactor automatically all the twisterlib.py. I'm fixing this now fixed

@PerMac PerMac force-pushed the enchancement/quarantine_list branch from cac6e3e to 3ad3bfe Compare Mar 25, 2021
@nashif
Copy link
Member

nashif commented Mar 29, 2021

if this is ready for review, move it out of draft please.

@PerMac PerMac force-pushed the enchancement/quarantine_list branch 2 times, most recently from a56b603 to 4613ce7 Compare Apr 1, 2021
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>
@PerMac PerMac force-pushed the enchancement/quarantine_list branch from 4613ce7 to 8a4606d Compare Apr 1, 2021
@PerMac PerMac marked this pull request as ready for review Apr 1, 2021
@PerMac
Copy link
Member Author

PerMac commented Apr 1, 2021

@nashif ready for review

@hakehuang hakehuang self-requested a review Apr 2, 2021
Copy link
Member

@nashif nashif left a comment

minor, otherwise lgtm

@@ -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
+++++++++++++++++++++++++++
Copy link
Member

@nashif nashif Apr 9, 2021

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.

Copy link
Member Author

@PerMac PerMac Apr 12, 2021

Choose a reason for hiding this comment

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

fixed

@nashif nashif self-assigned this Apr 10, 2021
Add section about quarantine feature

Signed-off-by: Maciej Perkowski <Maciej.Perkowski@nordicsemi.no>
@PerMac PerMac force-pushed the enchancement/quarantine_list branch from 5268522 to 9197130 Compare Apr 12, 2021
@PerMac PerMac requested a review from nashif Apr 12, 2021
nashif
nashif approved these changes Apr 12, 2021
@nashif nashif merged commit 22a82bc into zephyrproject-rtos:master Apr 12, 2021
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

twister: Add option to load list with quarantined tests
5 participants