-
Notifications
You must be signed in to change notification settings - Fork 27
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): record app_path in JUnit reports #336
feat(junit): record app_path in JUnit reports #336
Conversation
Hi @hfudev . Please take a look at the changes. Thanks! |
124b8bc
to
0650486
Compare
Shall we also add a test for |
2a95e62
to
381db08
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall LGTM. a few comments posted
# Add the 'pytest_case_name' attribute for each test case to enable later extraction | ||
# of the app_path from the pytest_case_name to app_path mapping | ||
if 'pytest_case_name' not in testcase.attrib: | ||
testcase.attrib['pytest_case_name'] = testcase.attrib.get('name') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question, why can't we use the attrib['name']
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that in some cases, the pytest test case name is being replaced by the C test case name, resulting in the name being absent in the mapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. so I have a different implementation of the whole task.
single-dut test cases
instead of your current modify main junit report way, we could record the app_path
directly. for example,
in
self.testsuite.add_unity_test_cases(log) |
it could be self.testsuite.add_unity_test_cases(log, test_case_name=self.test_case_name
, which writes the test_case_name
to pytest_case_name
multi-dut test cases
we could modify this function instead
def process_raw_report_data(self, raw_data_to_report) -> t.Dict: |
to insert the pytest_case_name
by these approaches, the data we recorded in the objects is correct. we don't have to generate the wrong one first, then write a function to replace the wrong one into correct ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this suggestion isn't viable. For example, self.testsuite.add_unity_test_cases
is executed only within the expect_unity_test_output
context. However, there are numerous scenarios where users call expect_exact
. Additionally, I tested it on the provided tests in this PR, and in both cases, the proposed solution either fails to work or requires significantly more changes compared to the current implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this work in #338, please take a look. I'll close this PR.
381db08
to
4191a35
Compare
Description
This PR introduces the feature to record attribute
app_path
in JUnit reports.Related
Closes #332
Testing
Added test that ensures that the
app_path
attribute is correctly recorded for each<testcase>
element in the JUnit report.Checklist
Before submitting a Pull Request, please ensure the following: