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

[RFC] pytestplugin: make "target" a parametrized fixture #1554

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

QSchulz
Copy link

@QSchulz QSchulz commented Dec 3, 2024

Description

This allows users to run their pytest test scripts with an LG_ENV/--lg-env that has multiple targets instead of forcing the user to define one named "main" in order to run the tests.

One of the downsides of this implementation is that autogenerated doc wouldn't define the "target" fixture anymore as it is entirely programmatically defined.

I'm investigating using labgrid for validating U-Boot with multiple pytest tests inside a single file, for multiple targets. Not sure it's the best practice but I intuitively went for that.

Essentially:

---
targets:
    px30-ringneck-1:
        features:
            - something
        options:
            emmc_path: '/mmc@ff370000'
    [...]
    rk3399-puma-1:
        features:
            - bootablespi
            - somethingelse
        options:
            emmc_path: '/mmc@ff390000'
    [...]
# bootloader_command fixture depends on target fixture

def test_uboot_initial(bootloader_flash, bootloader_command):
    stdout = bootloader_command.run_check('version')
    assert 'U-Boot' in '\n'.join(stdout)


@pytest.mark.lg_feature('bootablespi')
def test_uboot_properfromspi(bootloader_flash, bootloader_command, env, target):
    [...]

I believe this would allow users to have one big YAML file for all their targets, or merge YAML files for the same target into one YAML file so with one call to pytest, multiple targets can be checked. It would also allow to have one big pytest file with all tests to run on all targets, with parametrizing their arguments via target:options/target:features.

I checked the output of pytest --lg-env local.yaml --collect-only test.py and the tests are listed twice, once for each target in the YAML file. I then ran the tests and because I don't have enough HW for the infra for two boards, it complained for the second board that it was lacking resources, but the first board ran the tests just fine.

Checklist

  • Documentation for the feature (N/A?)
  • Tests for the feature (N/A?)
  • PR has been tested (somewhat; I don't have enough HW for the setup of two boards but the second board complains about lacking resources when I run pytest)

RFC because maybe there was a reason for only allowing a "main"-named target to run the pytest tests and also because I wasn't really able to make sure the second target was used for the second test run (though there are good hints it is).

Also I believe this may be a bit too enthusiastic, we probably want to add a selection mechanism instead of simply adding all targets from the file? e.g. maybe add an --lg-target option which can be repeated to collect which targets to include.

Also not entirely sure which part of the documentation we would need to update for that.

This allows users to run their pytest test scripts with an
LG_ENV/--lg-env that has multiple targets instead of forcing the user to
define one named "main" in order to run the tests.

One of the downsides of this implementation is that autogenerated doc
wouldn't define the "target" fixture anymore as it is entirely
programmatically defined.

Signed-off-by: Quentin Schulz <[email protected]>
Copy link

codecov bot commented Dec 3, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Project coverage is 56.4%. Comparing base (b67bfc1) to head (dec056e).

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
labgrid/pytestplugin/fixtures.py 84.6% 2 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##           master   #1554   +/-   ##
======================================
  Coverage    56.3%   56.4%           
======================================
  Files         169     169           
  Lines       13178   13185    +7     
======================================
+ Hits         7432    7438    +6     
- Misses       5746    5747    +1     
Flag Coverage Δ
3.10 56.4% <85.7%> (+<0.1%) ⬆️
3.11 56.4% <85.7%> (+<0.1%) ⬆️
3.12 56.4% <85.7%> (+<0.1%) ⬆️
3.13 56.3% <85.7%> (+<0.1%) ⬆️
3.9 56.4% <85.7%> (+<0.1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jremmet
Copy link
Contributor

jremmet commented Dec 3, 2024

We are also interested to join yaml configs of different boards, but selecting would be better approach for our usecase.
env.get_target() is already prepared for a resource name.

@jluebbe
Copy link
Member

jluebbe commented Dec 3, 2024

I'm investigating using labgrid for validating U-Boot with multiple pytest tests inside a single file, for multiple targets. Not sure it's the best practice but I intuitively went for that.

Essentially:

---
targets:
    px30-ringneck-1:
        features:
            - something
        options:
            emmc_path: '/mmc@ff370000'
    [...]
    rk3399-puma-1:
        features:
            - bootablespi
            - somethingelse
        options:
            emmc_path: '/mmc@ff390000'
    [...]

I believe this would allow users to have one big YAML file for all their targets, or merge YAML files for the same target into one YAML file so with one call to pytest, multiple targets can be checked. It would also allow to have one big pytest file with all tests to run on all targets, with parametrizing their arguments via target:options/target:features.

RFC because maybe there was a reason for only allowing a "main"-named target to run the pytest tests and also because I wasn't really able to make sure the second target was used for the second test run (though there are good hints it is).

The use-case for defining multiple targets in the same environment is tests which need to access multiple targets. For example one target creates a WiFi AP and the other target connects to it. If you have one set of tests (perhaps generalized via the target:options/features mechanism) that you want to run on multiple different boards, use multiple environment yaml files.

If you'd use a parameterized fixture, pytest (at least without xdist, which isn't really compatible with labgrid) would run all test sequentially, although there is no conflict between tests on different targets. With one environment file per test setup (so only one target in the common case), you can simply run multiple pytest process in parallel (e.g. via a CI runner).

Furthermore, by not mixing multiple unrelated targets in one file, it's easy to see everything that's need for a test to run. In the wifi test example above, you'd maybe have one env for CI and another one for your personal lab at home. Both would use the same target names, so the differences would be abstracted for the pytest code. You'd define your own pair of target fixtures in conftest.py (e.g. target_ap and target_client).

What could certainly be improved is a way to specify which remote place to use in the environment, perhaps via reservations and tags. That would allow using the same environment file with multiple labs, at least if the places were built similarly enough.

@QSchulz
Copy link
Author

QSchulz commented Dec 3, 2024

What could certainly be improved is a way to specify which remote place to use in the environment, perhaps via reservations and tags. That would allow using the same environment file with multiple labs, at least if the places were built similarly enough.

Not sure what you mean here as there's already:

---
targets:
  main:
    resources:
      RemotePlace:
        name: 'my-place'

And you can use templating with LG_PLACE if desired.

I guess we can close this PR then as I understand this is clearly not a fit in labgrid's architecture.

I don't like that the target needs to be named "main" and I think we could make this selectable or define it in the environment file itself, e.g.:

---
dut: ringneck-px30_1
targets:
  ringneck-px30_1:
  [...]
  wifi-ap_1:
  [...]

I'm not sure if the name of the target is used in error messages but if we do, having "main" in all environment files seem a bit counterproductive for debugging.

But also, this has nothing to do with this MR and it's not a blocker for me so probably won't work on it unless it bothers me a lot in the future :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants