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(junit-merger): add flag to preserve Python test cases and "is_unity_case" attribute #324

Conversation

alekseiapa
Copy link
Collaborator

@alekseiapa alekseiapa commented Nov 29, 2024

Description

New Features

  1. Introduced the --unity-test-report-mode Flag

    • Behavior:
      • When enabled, Python test cases are retained in the JUnit report alongside detailed test cases (e.g., C test cases).
      • By default, Python test cases are replaced by detailed test cases in the report.
    • Purpose:
      • Helps maintain clarity by allowing both Python and C test cases in the report.
  2. Added the is_unity_case Attribute to Test Cases

    • Details:
      • is_unity_case="1": Indicates a Python test case.
      • is_unity_case="0": Indicates a non-Python test case (e.g., C test case).
    • Impact:
      • Improves clarity and traceability of test cases in the JUnit report.

Bug-fixes

  1. Fixed Inconsistent line and file Attributes in XML Reports
    • Removed [dut-X] prefixes from line and file attributes in the XML report for clarity.
    • Before:
      <testcase file="[dut-1]: ./main/case.c" line="[dut-1]: 42" />
    • After:
      <testcase file="./main/case.c" line="42" />

Example Usage

Default Behavior (Without Flag)

  • Python test cases are replaced by detailed test cases.
<testcase is_unity_case="0" file="./main/case.c" line="42" />
  • With --unity-test-report-mode:
<testcase is_unity_case="1" classname="test_python" name="test_case_1" />
<testcase is_unity_case="0" file="./main/case.c" line="42" />

Related

No related issues

Testing

Environment

  • Tested in a clean virtual environment
  1. Default Behavior (Without --unity-test-report-mode):

    • Verified Python test cases are replaced by detailed test cases.
    • Checked is_unity_case="0" for all non-Python test cases.
  2. With --unity-test-report-mode Flag:

    • Ensured Python test cases are retained with is_unity_case="1".
    • Confirmed non-Python test cases have is_unity_case="0".
  3. XML Validation:

    • Verified proper formatting of file and line attributes (removed [dut-X]).
    • Checked all test cases include the is_unity_case attribute.

Checklist

Before submitting a Pull Request, please ensure the following:

  • 🚨 This PR does not introduce breaking changes.
  • All CI checks (GH Actions) pass.
  • Documentation is updated as needed.
  • Tests are updated or added as necessary.
  • Code is well-commented, especially in complex areas.
  • Git history is clean — commits are squashed to the minimum necessary.

@alekseiapa alekseiapa force-pushed the fix/remove-dut-prefix-from-pytest-xml-attrib-values branch from 5bc9a36 to e9d6504 Compare November 29, 2024 08:35
@alekseiapa
Copy link
Collaborator Author

Hi @hfudev . could you please help with the review of the current PR? The test cases failing in the CI appear unrelated to the changes in this PR. It seems they are also failing on the main branch.

@alekseiapa alekseiapa self-assigned this Nov 29, 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.

@alekseiapa Thanks for the PR! Left a few comments.

pytest-embedded/pytest_embedded/unity.py Outdated Show resolved Hide resolved
pytest-embedded/pytest_embedded/plugin.py Outdated Show resolved Hide resolved
@alekseiapa alekseiapa force-pushed the fix/remove-dut-prefix-from-pytest-xml-attrib-values branch from e9d6504 to c400240 Compare November 29, 2024 10:50
@alekseiapa alekseiapa changed the title feat(junit-merger): add flag to preserve Python test cases and PYTHON_FUNC attribute feat(junit-merger): add flag to preserve Python test cases and "is_unity_case" attribute Nov 29, 2024
@alekseiapa
Copy link
Collaborator Author

Hi @hfudev . Could you please have a brief look one more time ? Thanks!

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. only a few nitpicks about coding styles

pytest-embedded/pytest_embedded/unity.py Outdated Show resolved Hide resolved
pytest-embedded/pytest_embedded/plugin.py Outdated Show resolved Hide resolved
pytest-embedded-idf/tests/test_idf.py Outdated Show resolved Hide resolved
@hfudev
Copy link
Member

hfudev commented Nov 29, 2024

besides, since we're generating the release notes from the commit history, better to split up the commit to two:

  • fix(unity): remove [dut-X] prefix from "line" and "file" attribute values in JUnit report
  • feat(unity): add flag to ...

@alekseiapa alekseiapa force-pushed the fix/remove-dut-prefix-from-pytest-xml-attrib-values branch 2 times, most recently from 655e07f to d9b482b Compare November 29, 2024 12:44
@alekseiapa
Copy link
Collaborator Author

Hi @hfudev . One more time please ? :)

@alekseiapa alekseiapa force-pushed the fix/remove-dut-prefix-from-pytest-xml-attrib-values branch from d9b482b to b415e1a Compare November 29, 2024 12:48
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.

sorry to be picky. one more round :D

pytest-embedded/pytest_embedded/plugin.py Outdated Show resolved Hide resolved
pytest-embedded/pytest_embedded/unity.py Outdated Show resolved Hide resolved
@alekseiapa alekseiapa force-pushed the fix/remove-dut-prefix-from-pytest-xml-attrib-values branch from b415e1a to e3856b5 Compare November 29, 2024 13:11
@alekseiapa
Copy link
Collaborator Author

@hfudev one more attempt :)

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.

Many thanks. I'll merge this PR and fix these two nitpicks with other small fixes later in my PR

@@ -111,6 +111,16 @@ def pytest_addoption(parser):
help='y/yes/true for True and n/no/false for False. '
'Set to True to prettify XML junit report. (Default: False)',
)
parser.addoption(
'--unity-test-report-mode',
choices=[UnityTestReportMode.REPLACE.value, UnityTestReportMode.MERGE.value],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
choices=[UnityTestReportMode.REPLACE.value, UnityTestReportMode.MERGE.value],
choices=[v.value for v in UnityTestReportMode],

@@ -1183,7 +1193,9 @@ def unity_tester(dut: t.Union['IdfDut', t.Tuple['IdfDut']]) -> t.Optional['CaseT


def pytest_configure(config: Config) -> None:
config.stash[_junit_merger_key] = JunitMerger(config.option.xmlpath)
config.stash[_junit_merger_key] = JunitMerger(
config.option.xmlpath, config.getoption('unity_test_report_mode', default='replace')
Copy link
Member

Choose a reason for hiding this comment

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

missed one

@alekseiapa
Copy link
Collaborator Author

Many thanks. I'll merge this PR and fix these two nitpicks with other small fixes later in my PR

Thank you for the review !

@hfudev hfudev merged commit 96809ea into espressif:main Nov 29, 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.

2 participants