Skip to content

Conversation

@lylezhu2012
Copy link
Contributor

@lylezhu2012 lylezhu2012 commented Apr 3, 2025

Add dedicated test case bluetooth.classic.sdp.server.no_blobs and bluetooth.classic.sdp.client.no_blobs with the extra argument CONFIG_BUILD_ONLY_NO_BLOBS=y and build_only: true to make sure the tests sdp_s and sdp_c can be passed by Zephyr CI.

Fixes #88060.

makeshi
makeshi previously approved these changes Apr 3, 2025
MarkWangChinese
MarkWangChinese previously approved these changes Apr 3, 2025
Copy link
Member

@jhedberg jhedberg left a comment

Choose a reason for hiding this comment

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

This doesn't seem like a good solution, since the tests are not specific to platforms that require blobs, rather they're generically named, implying that they should be runnable on any platform supporting Bluetooth Classic. You also have native_sim listed as a supported platform, which definitely does not require any blobs. I'd also expect the BUILD_ONLY_NO_BLOBS to always be combined with ‘build_only: true‘ in the test definition.

@nashif is there any recommended way to handle situations like this?

@lylezhu2012
Copy link
Contributor Author

This doesn't seem like a good solution, since the tests are not specific to platforms that require blobs, rather they're generically named, implying that they should be runnable on any platform supporting Bluetooth Classic. You also have native_sim listed as a supported platform, which definitely does not require any blobs. I'd also expect the BUILD_ONLY_NO_BLOBS to always be combined with ‘build_only: true‘ in the test definition.

@nashif is there any recommended way to handle situations like this?

@nashif do you have any comments/suggestions?

@lylezhu2012 lylezhu2012 requested a review from nashif April 9, 2025 06:58
@lylezhu2012
Copy link
Contributor Author

@jhedberg Is it possible to make PR move forward?

@jhedberg
Copy link
Member

jhedberg commented Apr 9, 2025

@jhedberg Is it possible to make PR move forward?

I tried asking for help on #pr-help a few days ago. Next option could be to add the dev-review label and discuss this in tomorrow's meeting.

@lylezhu2012 lylezhu2012 added the dev-review To be discussed in dev-review meeting label Apr 9, 2025
@lylezhu2012
Copy link
Contributor Author

Next option could be to add the dev-review label and discuss this in tomorrow's meeting.

I added label dev-review. Thanks a lot.

@jhedberg jhedberg removed the dev-review To be discussed in dev-review meeting label Apr 10, 2025
@jhedberg
Copy link
Member

Initial consensus from Bluetooth WG: define a separate test case for the blobs configuration

@lylezhu2012 lylezhu2012 dismissed stale reviews from MarkWangChinese and makeshi via 9de1c65 April 10, 2025 13:26
@lylezhu2012 lylezhu2012 force-pushed the tests_br_sdp_c_s_building_issue branch from b732081 to 9de1c65 Compare April 10, 2025 13:26
@lylezhu2012
Copy link
Contributor Author

I added test cases bluetooth.classic.sdp.server.no_blobs and bluetooth.classic.sdp.client.no_blobs.

@jhedberg , Please help review my changes.

@lylezhu2012 lylezhu2012 force-pushed the tests_br_sdp_c_s_building_issue branch from 9de1c65 to 64fe57d Compare April 14, 2025 02:11
@lylezhu2012 lylezhu2012 requested a review from jhedberg April 14, 2025 02:17
@lylezhu2012 lylezhu2012 changed the title tests: Bluetooth: Classic: Add extra arg CONFIG_BUILD_ONLY_NO_BLOBS=y tests: Bluetooth: Classic: Add dedicated test case for no_blobs Apr 14, 2025
@lylezhu2012 lylezhu2012 force-pushed the tests_br_sdp_c_s_building_issue branch from 64fe57d to 656dabd Compare April 16, 2025 02:25
Comment on lines 21 to 24
harness: pytest
harness_config:
pytest_dut_scope: session
fixture: usb_hci
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious, what does the harness information mean for a test that's only supposed to be built but never run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I know, the test case with fixture will not be run.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but what does it mean? The test case says build_only: true, so what use is the fixture definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

build_only: true and CONFIG_BUILD_ONLY_NO_BLOBS=y are only used for CI. For testing on hardware, all of these two tags need to be removed.

So I would like to keep all tags.

Copy link
Member

Choose a reason for hiding this comment

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

Can't you use the generic test case then? You can use --force-platform (or -K) with twister to run it on another platform than what's explicitly listed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the test case bluetooth.classic.sdp.client.no_blobs is only used to verify the building issue of the platform mimxrt1170_evk@B/mimxrt1176/cm7 by CI?
Thus the following tags need to be removed from the test case bluetooth.classic.sdp.client.no_blobs.

    harness: pytest
    harness_config:
      pytest_dut_scope: session
      fixture: usb_hci

And test case bluetooth.classic.sdp.client is used to run test the test case on hardware. Right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed harness and harness_config tags from test case bluetooth.classic.sdp.client.no_blobs

@lylezhu2012 lylezhu2012 requested a review from jhedberg April 17, 2025 06:24
Add dedicated test case `bluetooth.classic.sdp.server.no_blobs` and
`bluetooth.classic.sdp.client.no_blobs` with the extra argument
`CONFIG_BUILD_ONLY_NO_BLOBS=y` and `build_only: true` to make sure the
tests sdp_s and sdp_c can be passed by Zephyr CI.

Fixes zephyrproject-rtos#88060.

Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>
@kartben kartben merged commit c5429d3 into zephyrproject-rtos:main Apr 22, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tests: mimxrt1170_evk@B/mimxrt1176/cm7: tests.bluetooth.classic.sdp_s build failure

5 participants