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

refactor: change logic of threading control in CaseTester for MultiDev (RDT-534) #223

Merged
merged 2 commits into from
Nov 23, 2023

Conversation

horw
Copy link
Collaborator

@horw horw commented Jul 26, 2023

Replaced logic of creating thread
before: new case every time create new thread
now: create thread once, and then thread wait for task
closes #184
closes #162

@horw horw force-pushed the ref/thread_in_MultiDevCaseTester branch from b9db548 to d693f1e Compare July 26, 2023 09:00
@horw horw force-pushed the ref/thread_in_MultiDevCaseTester branch 2 times, most recently from 8955ebd to 19db0cb Compare August 25, 2023 07:07
@horw
Copy link
Collaborator Author

horw commented Aug 25, 2023

Hello, I've refactored thread depends code, now it works with generator functions

@github-actions github-actions bot changed the title refactor: change logic of threading control in CaseTester for MultiDev refactor: change logic of threading control in CaseTester for MultiDev (RDT-534) Aug 25, 2023
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.

LGTM! The new approach looks good.

Still got some high-level problem.

The merged junit xml looks like this:

<testcase file="[group dev-0]: components/driver/test_apps/spi/master/main/test_spi_master.c&amp;lt;-------------------&amp;gt;&#10;[group dev-1]: components/driver/test_apps/spi/master/main/test_spi_master.c" line="[group dev-0]: 1603&amp;lt;-------------------&amp;gt;&#10;[group dev-1]: 1603" time="1.859" name="('esp32c6', 'esp32c6').('defaults', 'defaults').SPI_Master:Test multiple devices [dut-0]">
            <system-out>[group dev-0]:
                ... log ...
                &amp;lt;-------------------&amp;gt;
                [group dev-1]:
                ... log ...
        </testcase>

Got two places to improve:

  1. [group dev-0] should be [dut-0] to keep consistence
  2. The ---------- separator should not be used in attributes, like file, line

@hfudev
Copy link
Member

hfudev commented Sep 1, 2023

@ginkgm @L-KAYA PTAL as well.

After this PR got merged, the old case_tester used in ESP-IDF could be replaced by this unity_tester

The benefits of this approach are:

  1. merged junit reports, with stdout (including performance data)
  2. correct test case name even app got panic'ed
  3. remove thread/process. Support multi OS.

@L-KAYA
Copy link
Contributor

L-KAYA commented Sep 1, 2023

Have went through the code, I love the idea of iterator! LGTM!

@horw horw requested a review from hfudev September 4, 2023 07:03
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.

LGTM. Let's drop the deprecation warning first, usually it's not a good idea to confuse users...

For the optimization of the timeout calculation, you may do it in a separate PR.

pytest-embedded-idf/pytest_embedded_idf/unity_tester.py Outdated Show resolved Hide resolved
pytest-embedded-idf/pytest_embedded_idf/unity_tester.py Outdated Show resolved Hide resolved
pytest-embedded-idf/pytest_embedded_idf/unity_tester.py Outdated Show resolved Hide resolved
@horw horw force-pushed the ref/thread_in_MultiDevCaseTester branch from 472fd39 to 3038131 Compare September 19, 2023 05:25
@horw
Copy link
Collaborator Author

horw commented Sep 19, 2023

FAILED pytest-embedded-idf/tests/test_idf.py::test_dut_run_all_single_board_cases - AssertionError: assert 10.896 < 10.1

I've got this error in the pipeline, will check for it

p.s. problem was with device reset

@horw horw force-pushed the ref/thread_in_MultiDevCaseTester branch from 765016a to 56de649 Compare September 19, 2023 06:10
@hfudev
Copy link
Member

hfudev commented Sep 19, 2023

FAILED pytest-embedded-idf/tests/test_idf.py::test_dut_run_all_single_board_cases - AssertionError: assert 10.896 < 10.1

I've got this error in the pipeline, will check for it

p.s. problem was with device reset

oh, somehow I missed it in the review. Glad you found it :)

@hfudev hfudev force-pushed the ref/thread_in_MultiDevCaseTester branch from 56de649 to 89bae77 Compare October 30, 2023 13:24
@horw horw force-pushed the ref/thread_in_MultiDevCaseTester branch 2 times, most recently from 89bae77 to 7d0ee35 Compare November 14, 2023 02:19
@horw horw force-pushed the ref/thread_in_MultiDevCaseTester branch from 1b3c212 to 3529f27 Compare November 14, 2023 02:56
@hfudev hfudev merged commit e483289 into espressif:main Nov 23, 2023
4 of 5 checks passed
@hfudev
Copy link
Member

hfudev commented Nov 23, 2023

Thank you @horw LGTM!

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