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: shell harness with commands alongside the harness_config #85612

Merged

Conversation

gchwier
Copy link
Collaborator

@gchwier gchwier commented Feb 11, 2025

Allow user to add shell commands in testcase/sample yaml file alongside the harness_config like in the console harness.

ref: #85033

Instead of having shell commands in a separate file, one can add:

        harness_config:
          shell_commands:
          - command: "kernel cycles"
            expected: "cycles: .* hw cycles"
          - command: "kernel version"
            expected: "Zephyr version .*"
          - command: "kernel sleep 100"

Added examples in samples/subsys/testsuite/pytest/shell and updated existing samples.
To test you can run:

$ZEPHYR_BASE/scripts/twister -vv -T samples/subsys/testsuite/pytest/shell -G --no-clean -ll debug

$ZEPHYR_BASE/scripts/twister -vv -T samples/drivers/flash_shell -p native_sim -c -ll debug

$ZEPHYR_BASE/scripts/twister -vv -T samples/subsys/shell/shell_module -s sample.shell.shell_module -p native_sim -c -ll debug

@nashif , @bjarki-andreasen answering that comment:
#85033 (comment)
in one sample I added example usage of anchors in yaml files, it works!

  sample.harness.shell:
    harness_config:
      shell_commands: &kernel_commands     <-------
      - command: "kernel cycles"
        expected: "cycles: .* hw cycles"
      - command: "kernel version"
        expected: "Zephyr version .*"
      - command: "kernel sleep 100"
  sample.harness.shell.vt100_colors_off:
    harness_config:
      shell_commands: *kernel_commands   <------

@@ -107,9 +107,17 @@ schema;scenario-schema:
type: map
required: false
mapping:
"shell_params_file":
Copy link
Member

Choose a reason for hiding this comment

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

why is this being removed? what if I want to have multiple scenarios with different files, i.e not using the default test_shell.yml?

Copy link
Collaborator Author

@gchwier gchwier Feb 11, 2025

Choose a reason for hiding this comment

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

You can have multiple test configurations with different shell commands in harness_config. However I understand the scenario, to override during testing the test_shell.yml (but you can also change testcase.yml).

I can restore that option to load command from given file or from harness_config.

I though also about more generic approach, to be able to include yaml snippet, e.g. using https://pypi.org/project/pyyaml-include/, than you can keep test configurations in separate files if testcase yaml is to large.

Copy link
Member

@nashif nashif Feb 11, 2025

Choose a reason for hiding this comment

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

to override during testing the test_shell.yml (but you can also change testcase.yml).

not sure what you mean here, this is not just during testing...

test1:
  harness_config:
    shell_param_file: x.yml
test2:
  harness_config:
    shell_param_file: y.yml

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so as I wrote: You can have multiple test configurations with different shell commands in harness_config,
it can be replaced with:

test1:
  harness_config:
    shell_commands:
      - command1
        expected1
      - command2
test2:
  harness_config:
    shell_commands:
      - command2
        expected2
      - command3
        expected3

IMO keeping commands in a separate YAML file introduces unnecessary complexity. It will make it harder to understand what is going on in the tests. It's better to change it now, while no tests are using this feature. Later, we will need to consider backward compatibility.

@gchwier gchwier force-pushed the grch-shell-harness-commands branch 2 times, most recently from 8adb3aa to 0a5fd8a Compare February 11, 2025 15:11
de-nordic
de-nordic previously approved these changes Feb 11, 2025
Copy link
Collaborator

@de-nordic de-nordic left a comment

Choose a reason for hiding this comment

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

Flash shell looks ok.

Copy link
Collaborator

@bjarki-andreasen bjarki-andreasen left a comment

Choose a reason for hiding this comment

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

I like it :)

Copy link
Member

@nashif nashif left a comment

Choose a reason for hiding this comment

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

need to add back reading test data from files. As I said here: #85033 (comment) and here #85033 (comment) I can live with testdata in the testcase.yaml file for small sets, but this needs to scale beyond just a few entries and testdata in general should be seperated from test definition and configuration.

Now it basically supports reading from one single file that is pre-defined, which makes it worse that it was before.

@gchwier gchwier dismissed stale reviews from bjarki-andreasen and de-nordic via a29dd7e February 12, 2025 17:27
@gchwier gchwier force-pushed the grch-shell-harness-commands branch from 0a5fd8a to a29dd7e Compare February 12, 2025 17:27
@gchwier
Copy link
Collaborator Author

gchwier commented Feb 12, 2025

restored option to read commands from file. Changed shell_params_file name to shell_commands_file.
And for current implementation shell_commands option takes precedence over shell_commands_file.

@nashif Please review

@gchwier gchwier force-pushed the grch-shell-harness-commands branch 3 times, most recently from 95a0450 to 053ae1c Compare February 13, 2025 09:10
nashif
nashif previously approved these changes Feb 13, 2025
@gchwier
Copy link
Collaborator Author

gchwier commented Feb 13, 2025

rebased and added new lines to twister.rst after code-block... some problems with building docs :(

@gchwier gchwier force-pushed the grch-shell-harness-commands branch from 4f8b4bd to e467959 Compare February 13, 2025 12:50
hakehuang
hakehuang previously approved these changes Feb 13, 2025
@gchwier gchwier force-pushed the grch-shell-harness-commands branch from e467959 to 2ba3b85 Compare February 14, 2025 08:49
yperess
yperess previously approved these changes Feb 14, 2025
@gchwier
Copy link
Collaborator Author

gchwier commented Feb 14, 2025

I don't really know why it is failing on sample.pytest.shell.vt100_colors_off on qemu_cortex_a53/qemu_cortex_a53/smp,
locally it works

@nashif
Copy link
Member

nashif commented Feb 14, 2025

I don't really know why it is failing on sample.pytest.shell.vt100_colors_off on qemu_cortex_a53/qemu_cortex_a53/smp,
locally it works

smp is sporadic in CI (and locally), I would just exclude this one or we will be hitting it in every other PR.

Allow user to add shell commands in testcase/sample yaml file
alongside the harness_config like in the console harness.

Signed-off-by: Grzegorz Chwierut <[email protected]>
@gchwier gchwier dismissed stale reviews from yperess, hakehuang, and bjarki-andreasen via 6a90a07 February 14, 2025 13:01
@gchwier gchwier force-pushed the grch-shell-harness-commands branch from 2ba3b85 to 6a90a07 Compare February 14, 2025 13:01
@gchwier
Copy link
Collaborator Author

gchwier commented Feb 14, 2025

smp is sporadic in CI (and locally), I would just exclude this one or we will be hitting it in every other PR.

added not CONFIG_SMP in shell sample ... however I do not understand, where is the problem (failed only on qemu_cortex_a53/smp, with vt100_colors_off .. and cannot reproduce it on local machine)

@fabiobaltieri fabiobaltieri merged commit df40aa5 into zephyrproject-rtos:main Feb 14, 2025
36 checks passed
@gchwier gchwier deleted the grch-shell-harness-commands branch February 14, 2025 23:36
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.

9 participants