sys/shell: cmds_json builtin command#20964
Conversation
maribu
requested review from
MichelRottleuthner,
aabadie,
leandrolanzieri and
miri64
as code owners
maribu
requested review from
chrysn
and removed request for
MichelRottleuthner,
aabadie,
leandrolanzieri and
miri64
github-actions
bot
added
Area: tests
maribu
added
Type: enhancement
| 'end_test': 'ends a test', | ||
| 'echo': 'prints the input command', | ||
| 'empty': 'print nothing on command', | ||
| 'periodic': 'periodically print command', |
There was a problem hiding this comment.
This was not tested against before. The way it was implemented before it would skip over lines in-between the given list of output lines. The test is now a bit stricter in this regard.
maribu
force-pushed
the
tests/rust_libs/improve-test-robustness
branch
from
6ad4a52 to
c46c350
Compare
mguetschow
left a comment
mguetschow
left a comment
There was a problem hiding this comment.
Ah, found the right PR for my comments :P CC @Teufelchen1
makefiles/pseudomodules.inc.mk
Outdated
| PSEUDOMODULES += shell_cmd_vfs | ||
| PSEUDOMODULES += shell_cmds_default | ||
| PSEUDOMODULES += shell_hooks | ||
| PSEUDOMODULES += shell_json |
There was a problem hiding this comment.
Shouldn't this be rather shell_cmd_cmds_json to match the pattern of other shell command modules?
There was a problem hiding this comment.
I think that can be confusing, as the others are all externally implemented shell commands (they have a description and show up in help). This is however implemented as a builtin (see below for why) and behaves differently.
sys/shell/shell.c
Outdated
| else if (IS_USED(MODULE_SHELL_JSON) && !strcmp("cmds_json", argv[0])) { | ||
| print_commands_json(command_list); | ||
| } |
There was a problem hiding this comment.
Any reason apart from historical ones why both help and the new command are not defined using SHELL_COMMAND? This would avoid the special casing here and automatically include both in their outputs.
There was a problem hiding this comment.
The issue is that the need to access the first parameter of shell_run_forever(). Externally implemented ones would only be able to list XFA based shell commands without accesa to that argument.
There was a problem hiding this comment.
Oh, I see! So it's an interesting mix of statically available xfa shell command definitions and a dynamically passeable command list at runtime via shell_run_forever (and friends?).
Can you think of use cases where it is actually necessary to have the dynamic command list? Otherwise we could store it upon call to shell_run_forever as a global state.
There was a problem hiding this comment.
I guess that would probably be fine. I think eventually the use of non-XFA shell commands will pass away over time.
There was a problem hiding this comment.
But maybe something for another PR, I guess 👍
|
The CI complains about missing newlines in the python script: https://github.com/RIOT-OS/RIOT/actions/runs/11748909732/job/32734016006?pr=20964 |
This command does the same as `help`, but provides a machine readable JSON rather than a human readable table. It is only provided when the (pseudo-)module `shell_builtin_cmd_help_json` is used.
This increases the robustness of the test by not relying on the order shell commands are printed in. At least for XFA based shell commands, there is no guarantee in which order they will be shown in the help.
maribu
force-pushed
the
tests/rust_libs/improve-test-robustness
branch
from
9cead8a to
b605347
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Thx :) |
4 participants
Contribution description
This adds the
cmds_jsonbuiltin command that does the same ashelp, but provides a machine readable JSON rather than a human readable table. It is only provided when the (pseudo-)moduleshell_jsonis used.This in turn is used in a second commit in
tests/rust_libs. It would otherwise depend on the order in which shell commands are printed, which at least for XFA shell commands (as provided by rust) is not the case.Testing procedure
Green CI: The new builtin command is tested implicitly in
tests/rust_libs.Issues/PRs references
Needed to get #20958 passed.