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.

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

Jump to bottom

sanitycheck: improve fixture support and fix some bugs... #25524

Merged
merged 6 commits into from
May 24, 2020
Merged

sanitycheck: improve fixture support and fix some bugs... #25524

merged 6 commits into from
May 24, 2020

Conversation

nashif
Copy link
Member

@nashif nashif commented May 21, 2020

Fixes #24943
Fixes #25177

nashif added 4 commits May 21, 2020 10:36
@nashif
sanitycheck: cleanup fixture processing
74586a0 
Cleanup fixture processing and allow ztest testcases to support
harness_config with fixture definition.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
@nashif
tests: maxim_ds3231_api: use fixture
a05907e 
This test depends on additional hardware being connected to the board,
add a fixture and cleanup whitelist.

Fixes zephyrproject-rtos#25177

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
@nashif
sanitycheck: support fixtures
33b2086 
Add fixtures to hardware map schema.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
@nashif
sanitycheck: add fixture support to hardware map
aa39dc6 
It is now possible to add a list of fixture a platform supports which is
matched to testcases requesting fixtures to be avaiable to be able to
run.

For example:

- available: true
  connected: true
  id: 0240000026334e450015400f5e0e000b4eb1000097969900
  platform: frdm_k64f
  product: DAPLink CMSIS-DAP
  runner: pyocd
  serial: /dev/ttyACM9
  fixtures:
    - gpio_loopback

Fixes zephyrproject-rtos#24943

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
@nashif nashif requested review from mnkp and pabigot as code owners May 21, 2020 14:41
@nashif nashif requested a review from erwango May 21, 2020 14:45
@nashif nashif added the DNM This PR should not be merged (Do Not Merge) label May 21, 2020
@nashif
Copy link
Member Author

nashif commented May 21, 2020

I might need to split this PR to get the medium bug in first...

@zephyrbot zephyrbot added area: Tests Issues related to a particular existing or missing test area: Sanitycheck Sanitycheck has been renamed to Twister labels May 21, 2020
@zephyrbot
Copy link
Collaborator

zephyrbot commented May 21, 2020 •
edited
Loading

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@nashif
tests: gpio_basic: this test requires a fixture
9f8a7d4 
This test requires a fixture to be installed, in this case a wire
connecting two GPIO pins.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
@nashif nashif force-pushed the sanitycheck/fixture_support branch from bfe850a to 9f8a7d4 Compare May 21, 2020 15:00
pabigot
pabigot reviewed May 21, 2020
View reviewed changes
Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

I think we need better documentation on exactly what the fixture is and how you enable it. doc/guides/test/sanitycheck.rst just tells how to use it.

I infer these changes fix #25177 by keeping the sample board overrides from being built because the boards don't have the device attached. So the way to do testing requires adding the fixture to the board in the hardware yaml used for testing.

If so, that should be added to the documentation since there are no in-tree hardware yaml samples (AFAIK).

@nashif
Copy link
Member Author

nashif commented May 21, 2020

I infer these changes fix #25177 by keeping the sample board overrides from being built because the boards don't have the device attached.

the test will still be built even if you do not have the fixture attached, with --device-testing however, if there is no fixture attached to the board and you do not specify --fixture -r have it in the hardware map, it will not be built or run.

@nashif
Copy link
Member Author

nashif commented May 21, 2020

If so, that should be added to the documentation since there are no in-tree hardware yaml samples (AFAIK).

documentation for the HW map addition will be added, hence the DNM

pabigot
pabigot reviewed May 21, 2020
View reviewed changes
Copy link
Collaborator

@pabigot pabigot left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Seems to properly filter the gpio tests once unrelated problems are addressed (#25530, #25531).

Approval pending documentation.

erwango
erwango reviewed May 22, 2020
View reviewed changes
tests/drivers/gpio/gpio_basic_api/testcase.yaml
@@ -4,3 +4,6 @@ tests:
depends_on: gpio
min_flash: 34
filter: dt_compat_enabled("test,gpio_basic_api")
harness: ztest
harness_config:
fixture: gpio_loopback
Copy link
Member

Choose a reason for hiding this comment

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

This would deserve rewrite of the sanitycheck.rst, as it mentions things like fixture_gpio_loop, fixture_ble_fw
I prefer the one proposed in this PR indeed (w/o the 'fixture_' prefix)

@erwango
Copy link
Member

erwango commented May 22, 2020

This works great!
A couple of comments, aside from the sanitycheck doc that would require an update:

  • It seems possible to declare more than one fixture in a test.yaml, but they are treated w/ 'OR'. So test will be executed on boards that declare either fixture_a either fixture_b. I would have expected the fixture list to be treated with AND as we can imagine a test requiring several HW fixtures but having a test running on various fixtures seems harder to do. So if possible can you implement the AND behavior. In any case this should be documented.
  • I've tested and it's possible to add several fixtures to a board. So this is fine.
  • Not impacted by this PR, but direct consequence: I've tried to define a shield fixture and hit the following issue:
    ERROR - 'nucleo_wb55rg/samples/shields/x_nucleo_iks02a1/standard/sample.shields.x_nucleo_iks02a1.standard' generated an exception: module 'harness' has no attribute 'Shield'
    I managed to run the test by changing harness: shield to harness: ztest or harness: console. I'm not sure if this is expected as 'shield' seems compliant with 'harness' definition: Usually pertains to external dependency domains but can be anything such as console, sensor, net, keyboard, or Bluetooth.

@nashif
Copy link
Member Author

nashif commented May 22, 2020 

* It seems possible to declare more than one fixture in a test.yaml, but they are treated w/ 'OR'.

we should only have one fixture per testcase.

* Not impacted by this PR, but direct consequence: I've tried to define a shield fixture and hit the following issue:
  `ERROR   - 'nucleo_wb55rg/samples/shields/x_nucleo_iks02a1/standard/sample.shields.x_nucleo_iks02a1.standard' generated an exception: module 'harness' has no attribute 'Shield'`

This happens because we know nothing about the shield harness, this needs to be defined as a new class in sanitycheck. Since you have fixture now, you could change this to console or ztest and use the fixture keyword for filtering. We probably need to change all those testcases that have custom harness and use fixtures instead.

@nashif nashif removed the DNM This PR should not be merged (Do Not Merge) label May 22, 2020
@nashif nashif added this to the v2.3.0 milestone May 22, 2020
@nashif nashif force-pushed the sanitycheck/fixture_support branch from ffc279e to f0471b3 Compare May 22, 2020 11:37
@erwango
Copy link
Member

erwango commented May 22, 2020

This happens because we know nothing about the shield harness, this needs to be defined as a new class in sanitycheck. Since you have fixture now, you could change this to console or ztest and use the fixture keyword for filtering. We probably need to change all those testcases that have custom harness and use fixtures instead.

Sure. But then, harness definition may require an update, as it is more constrained than documented definition: Usually pertains to external dependency domains but can be anything such as console, sensor, net, keyboard, or Bluetooth.. Anyway this is not directly related to this PR. So approved

pabigot
pabigot requested changes May 22, 2020
View reviewed changes
Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

A couple minor doc tweaks.

It'd be nice if fixtures could be read from the board yaml as well as being described in the hardware device yaml. E.g. thingy52_nrf52832.yaml has:

supported:
  - i2c
  - hts221

so it should be possible to also specify that fixture: i2c_hts221 is available without having to add it by hand to the hardware map file.

That's an enhancement out of scope for this PR, IMO.

doc/guides/test/sanitycheck.rst Outdated Show resolved Hide resolved
doc/guides/test/sanitycheck.rst Outdated Show resolved Hide resolved
@nashif nashif force-pushed the sanitycheck/fixture_support branch from f0471b3 to 3739551 Compare May 22, 2020 12:21
pabigot
pabigot approved these changes May 22, 2020
View reviewed changes
doc/guides/test/sanitycheck.rst Outdated

When running `sanitycheck` with :option:`--device-testing`, the configured fixture
in the hardware map file will be matched to testcases requesting the same fixtures
and these test will be executed on the boards that provide this fixture.
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/test/tests/

@nashif nashif force-pushed the sanitycheck/fixture_support branch 2 times, most recently from ba9027d to 3f68fdb Compare May 22, 2020 12:49
@nashif
doc: sanitycheck: fixture documentation
66199af 
Add documentation about fixtures and how to use them in the hardware
map.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
@nashif nashif force-pushed the sanitycheck/fixture_support branch from 3f68fdb to 66199af Compare May 23, 2020 11:17
@carlescufi carlescufi merged commit ed44f05 into zephyrproject-rtos:master May 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PerMac PerMac PerMac approved these changes

@erwango erwango erwango approved these changes

@carlescufi carlescufi carlescufi approved these changes

@andrewboie andrewboie andrewboie approved these changes

@pabigot pabigot pabigot approved these changes

@mnkp mnkp Awaiting requested review from mnkp

@maksimmasalski maksimmasalski Awaiting requested review from maksimmasalski

@xuhao8210 xuhao8210 Awaiting requested review from xuhao8210

@chen-png chen-png Awaiting requested review from chen-png

Assignees
No one assigned
Labels
area: Documentation area: Sanitycheck Sanitycheck has been renamed to Twister area: Tests Issues related to a particular existing or missing test
Projects
None yet
Milestone
v2.3.0
7 participants
@nashif @zephyrbot @erwango @pabigot @andrewboie @carlescufi @PerMac