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

feat: create dut-factory method (RDT-587) #240

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

horw
Copy link
Member

@horw horw commented Nov 14, 2023

@horw
Copy link
Member Author

horw commented Nov 14, 2023

Hi @hfudev, this is a very initial draft commit for the implementation of the Fabrica class. I have a few questions. I have only created the base Dut class for now, but I don't think it's enough for the users. Should we implement other Duts in the same manner? (for example, IdfDut, SerialDut, etc.) It is possible, but it might result in a lot of duplicated code.

@github-actions github-actions bot changed the title feat: create dut-factory method (init commit) feat: create dut-factory method (init commit) (RDT-587) Nov 14, 2023
@horw
Copy link
Member Author

horw commented Nov 20, 2023

As I see this task, I will split it into the following tasks:

  • Move function logic under fixtures to dut_factory.
  • Test the moved solution.
  • Create a DutFactory class using these dut_factory functions.

@horw
Copy link
Member Author

horw commented Nov 21, 2023

It still seems rough, but it's starting to work. @hfudev, what do you think about doing it this way? Also, I commented on some issues here.

@horw horw force-pushed the feat/dut-factory branch 2 times, most recently from 00507eb to 71bdc44 Compare December 18, 2023 04:05
@horw
Copy link
Member Author

horw commented Dec 18, 2023

@hfudev, Hello, I have cleaned the history and it seems like the test was passed.

@hfudev
Copy link
Member

hfudev commented Dec 29, 2023

# test_foo.py
from pytest_embedded.dut_factory import DutFactory


def test_foo():
    dut = DutFactory.create(app_path='components/console/test_apps/console', target='esp32c6')
    dut.run_all_single_board_cases()

unfortunately it's not working with error message

            _pexpect_logfile = os.path.join(
>               buff_parametrized_fixtures['_meta'].logdir, f'custom-dut-{dut_index_global}.txt'
            )
E           KeyError: '_meta'

Seems like the set_buff_parametrized_fixtures will be triggered in another fixture parametrize_fixtures. If I don't use any fixture, the set_buff_parametrized_fixtures will be skipped.

and the embedded_services should also be able to pass in the DutFactory

for example

idf_qemu_dut = DutFactory.create(app_path='foo', target='esp32', embedded_services='qemu,idf')
idf_esp32_dut = DutFactory.create(app_path='foo', target='esp32', embedded_services='esp,idf')

Besides, could you also add the DutFactory to pytest_embedded/__init__.py __all__?

@horw horw force-pushed the feat/dut-factory branch 2 times, most recently from b1223d6 to 91063d4 Compare February 7, 2024 03:45
@horw
Copy link
Member Author

horw commented Feb 7, 2024

@hfudev
Thanks for your review! Your testcase was very useful!

This commit contains the following changes:

  • parametrize_fixtures changed to be autouse, which helps initialize set_buff_parametrized_fixtures.
  • Added unit_tester.
  • Added a test for esp,idf.
  • Call DutFactory.close() after every test, not the session.

Do you have any other testcase I can try to implement?

Further steps:

  • Add the DutFactory to pytest_embedded/__init__.py __all__.
  • Check dut_factory again, as there was some conflict related to the update in plugin.py when rebasing (I have just accepted my changes).
  • Check for possible issues with dut-{number}.

@horw horw force-pushed the feat/dut-factory branch 2 times, most recently from daf3d45 to c2d13ef Compare February 9, 2024 02:20
@horw horw force-pushed the feat/dut-factory branch 6 times, most recently from 100f052 to becb5df Compare April 2, 2024 08:43
@horw horw force-pushed the feat/dut-factory branch from 2f2b1da to becb5df Compare April 2, 2024 09:49
@hfudev hfudev force-pushed the feat/dut-factory branch from becb5df to 58626c6 Compare April 9, 2024 09:45
@horw horw changed the title feat: create dut-factory method (init commit) (RDT-587) feat: create dut-factory method (RDT-587) Apr 10, 2024
Copy link
Member

@hfudev hfudev left a comment

Choose a reason for hiding this comment

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

The code changes LGTM. Left some comments.

Besides of the "reverse dependency" issue, another question is that now we have to touch two places when we're introducing a new param right?

for example, if we support --esptool-version for `EspSerial, how many files shall we touch? (we won't support that, just an example) could you help document it somewhere in the comments as well?

pytest-embedded-idf/tests/test_idf.py Outdated Show resolved Hide resolved
pytest-embedded/pytest_embedded/dut_factory.py Outdated Show resolved Hide resolved
pytest-embedded/pytest_embedded/dut_factory.py Outdated Show resolved Hide resolved
pytest-embedded/pytest_embedded/dut_factory.py Outdated Show resolved Hide resolved
pytest-embedded/pytest_embedded/dut_factory.py Outdated Show resolved Hide resolved
@horw
Copy link
Member Author

horw commented Apr 16, 2024

for example, if we support --esptool-version for `EspSerial, how many files shall we touch? (we won't support that, just an example) could you help document it somewhere in the comments as well?

Hello, let's take wokwi_timeout as an example. This is the output from git grep wokwi_timeout. As a result, you need to make changes in dut_factory and the plugin.

pytest-embedded-wokwi/pytest_embedded_wokwi/wokwi_cli.py:41:        wokwi_timeout: t.Optional[int] = None,
pytest-embedded-wokwi/pytest_embedded_wokwi/wokwi_cli.py:57:        if (wokwi_timeout is not None) and (wokwi_timeout > 0):
pytest-embedded-wokwi/pytest_embedded_wokwi/wokwi_cli.py:58:            cmd.extend(['--timeout', str(wokwi_timeout)])
pytest-embedded/pytest_embedded/dut_factory.py:140:    wokwi_timeout,
pytest-embedded/pytest_embedded/dut_factory.py:291:                    'wokwi_timeout': wokwi_timeout,
pytest-embedded/pytest_embedded/dut_factory.py:584:        wokwi_timeout=0,
pytest-embedded/pytest_embedded/dut_factory.py:638:                'wokwi_timeout': wokwi_timeout,
pytest-embedded/pytest_embedded/plugin.py:950:def wokwi_timeout(request: FixtureRequest) -> t.Optional[str]:
pytest-embedded/pytest_embedded/plugin.py:952:    return _request_param_or_config_option_or_default(request, 'wokwi_timeout', None)
pytest-embedded/pytest_embedded/plugin.py:1010:    wokwi_timeout,

@horw horw force-pushed the feat/dut-factory branch 3 times, most recently from 21090bd to fdba35e Compare April 19, 2024 06:15
Copy link
Member

@hfudev hfudev left a comment

Choose a reason for hiding this comment

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

@horw Thanks for this great PR. Only some minor comments regarding to the code conventions. Let's merge it today!

pytest-embedded/pytest_embedded/dut_factory.py Outdated Show resolved Hide resolved
pytest-embedded/pytest_embedded/dut_factory.py Outdated Show resolved Hide resolved
pytest-embedded/pytest_embedded/dut_factory.py Outdated Show resolved Hide resolved
@horw horw force-pushed the feat/dut-factory branch 2 times, most recently from 8401b19 to e8f5c95 Compare April 23, 2024 10:02
@horw horw force-pushed the feat/dut-factory branch from e8f5c95 to 2744830 Compare April 23, 2024 10:04
@hfudev hfudev merged commit f538abd into espressif:main Apr 23, 2024
5 checks passed
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.

Make Create Dut Instance Easier (RDT-554)
2 participants