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

Update junit.py #3496

Closed
wants to merge 3 commits into from
Closed

Update junit.py #3496

wants to merge 3 commits into from

Conversation

shekharsorot
Copy link
Contributor

Context : for cases such as XFSTest, iPerf and others where each test case will have multiple nested cases controlled directly by the tool being called, we needed a way to cleanly pass subtests with messages, stacks and other information to Junit XML for parsing and possible injection into databases,

Added 3 new parameters.
include_passed_messages: ( Bool ) Include message and stacktrace in Junit XML output. Defaults to False
include_failed_messages: ( Bool ) Include message and stacktrace in Junit XML output. Defaults to True
include_skipped_messages: ( Bool ) Include message and stacktrace in Junit XML output. Defaults to True

Enhancements : Stacktrace now exists as an XML subproperty, making XML much cleaner.
If "message" and "stacktrace" properties are empty in the TestResultMessage object, its relevant subXML objects will not be added, makes XML parsing & viewing cleaner.

note: Ideally this change should not break existing Junit parsing scripts, but its expected that end users are using properly traversing XML tree to ensure they do not end up with an exeception when say..."message" attribute is missing because there was actually no message passed by the TestResultMessage object.

how to call ? Example code from runbook--

notifier:

  • type: html
  • type: env_stats
  • type: junit
    name: junit.lisa.xml
    include_subtest: true
    include_passed_messages: false
    include_failed_messages: true
    include_skipped_messages: true

Added 3 new parameters
    include_passed_messages:  ( Bool ) Include message and stacktrace in Junit XML output
    include_failed_messages: ( Bool ) Include message and stacktrace in Junit XML output
    include_skipped_messages: ( Bool ) Include message and stacktrace in Junit XML output

Enhancements : Stacktrace now exists as an XML subproperty, making XML much cleaner.

If "message" and "stacktrace" properties are empty  in the TestResultMessage object, its relevant subXML objects will not be added, makes XML parsing cleaner.

how to call ? Example code from runbook
notifier:
  - type: html
  - type: env_stats
  - type: junit
    name: junit.lisa.xml
    include_subtest: true
    include_passed_messages: false
    include_failed_messages: true
    include_skipped_messages: true
@shekharsorot shekharsorot marked this pull request as draft November 5, 2024 10:06
@shekharsorot shekharsorot marked this pull request as ready for review November 5, 2024 14:43
lisa/notifiers/junit.py Outdated Show resolved Hide resolved
# show passed case 'message' and 'stacktraces'
include_passed_messages: bool = False
# show 'message' for failed cases and 'stacktrace'
include_failed_messages: bool = True
Copy link
Member

Choose a reason for hiding this comment

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

When should it be set to False? If no scenario, it can be included always.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Default behavior is to include messages and stacktraces for Failed tests/subcases.
Ideally we want messages for skipped tests.
Passed tests/cases should be optional and extension can bundle additional information in stacktrace

Copy link
Member

Choose a reason for hiding this comment

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

I don't see the scenarios, which want set failed/skippped to false, and set passed to true. So the three flags are not needed. The logic always include stackstrace for failed and skipped, but never for passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

understood. I will modify code to only implement logic for successful test cases.

@@ -28,6 +28,12 @@ class JUnitSchema(schema.Notifier):
path: str = "lisa.junit.xml"
# respect the original behavior, include subtest by default
include_subtest: bool = True
# show passed case 'message' and 'stacktraces'
include_passed_messages: bool = False
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't need to be False by default, so the logic is more consistent for different types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I marked it false to ensure current output is not modified in any way. This is because i expect users to be parsing this output in their implementation of LISA.
Marking passed and skipped flag as false by default allows us to maintain some level of compat with existing implementation while allowing customization for new implementations.

Copy link
Member

Choose a reason for hiding this comment

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

If no scenario needs the stack trace for passed test cases, it doesn't need the flag.

lisa/notifiers/junit.py Outdated Show resolved Hide resolved
lisa/notifiers/junit.py Outdated Show resolved Hide resolved
@shekharsorot
Copy link
Contributor Author

shekharsorot commented Nov 25, 2024

Closing PR, will implement this as a private Patch instead as the changes are unique for XFStest instead of larger suite of existing test cases

@squirrelsc
Copy link
Member

@shekharsorot It's close to complete, please try to contribute to upstream to avoid extra cost on maintaining backports in 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.

2 participants