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

tests/sys/shell: use default terminal instead of socat #21058

Merged
merged 1 commit into from
Dec 4, 2024
Merged

tests/sys/shell: use default terminal instead of socat #21058

merged 1 commit into from
Dec 4, 2024

Conversation

mguetschow
Copy link
Contributor

Contribution description

tests/sys/shell currently per default uses socat as terminal, according to the comment to avoid having extra characters put into the IO by the terminal program. But that's exactly what make cleanterm is for, which is already used per default by the python testrunner.

I encountered issues with socat for this test (newlines not properly matched against) on boards that use usb_cdc_acm as stdio, while the approach with cleanterm works as expected.

Testing procedure

Run the test with some boards using make -C tests/sys/shell BOARD=xxx flash test. Successfully tested with native, nrf52840dk and feather-nrf52840-sense.

Issues/PRs references

The commit cdfece6 that introduced the change was merged one and a half months before the introduction of the cleanterm target in #12107

@mguetschow
tests/sys/shell: use default terminal instead of socat
be5d099 
python tests by default already use make cleanterm
@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Dec 2, 2024
@mguetschow mguetschow requested review from benpicco and maribu December 2, 2024 07:58
@maribu maribu added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: full build disable CI build filter CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: full build disable CI build filter labels Dec 2, 2024
@riot-ci
Copy link

riot-ci commented Dec 3, 2024

Murdock results

✔️ PASSED

be5d099 tests/sys/shell: use default terminal instead of socat

Success Failures Total Runtime
87 0 88 02m:07s

Artifacts

@maribu maribu enabled auto-merge December 4, 2024 00:43
@maribu maribu added this pull request to the merge queue Dec 4, 2024
Merged via the queue into RIOT-OS:master with commit f5b4b17 Dec 4, 2024
33 checks passed
@waehlisch waehlisch mentioned this pull request Dec 4, 2024
Closed
@mguetschow mguetschow deleted the tests-shell-socat branch December 4, 2024 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maribu maribu maribu approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@benpicco benpicco Awaiting requested review from benpicco

Assignees
No one assigned
Labels
Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mguetschow @riot-ci @maribu