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

[SUREFIRE-2219] Add test stdErr/stdOut output to ReportTestCase #692

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kriegaex
Copy link
Contributor

@kriegaex kriegaex commented Dec 4, 2023

TestSuiteXmlParser no longer ignores the system-out and system-err tags from the Surefire report XML. It parses them and sets the corresponding properties in ReportTestCase.

@michael-o, this is a bare PR for the back-end. It contains no tests and no changes to SurefireReportRenderer::constructTestCaseSection. I leave these up to you. But now you have the raw data to work from. Sorry, I do not have more time than this. Hope it helps a bit anyway. You can just commit on top of this draft PR.

To do:

  • unit tests
  • integration tests (make existing ones pass again, in case they are failing, add new ones)
  • add option to include console logs into the HTML test report
  • depending on the option, render monospaced code sections in ReportTestCase, representing System.out and System.err outputs

@kriegaex kriegaex marked this pull request as draft December 4, 2023 05:24
@michael-o
Copy link
Member

Thanks, I will look into this shortly. I also need to put other PRs into consider because of memory consumption, etc.

Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

At current glance looks totally reasonable. Kudos for the first time. Le me check the other open PRs which might intervene here.

@kriegaex
Copy link
Contributor Author

kriegaex commented Dec 6, 2023

I also need to put other PRs into consider because of memory consumption, etc.

Are we talking about memory consumption here or in the other PRs? What are you concerned about? The XML parser uses SAX, not a DOM. Of course, the console log contents need to be stored in ReportTestCase properties subsequently, which necessarily uses more memory than the non-existence of such properties. If you are very concerned about this, you could make populating these fields optional. But then,

  • either users need to set two properties in two different plugins to activate console log reporting for tests,
  • or you need to always report console log entries, if they exist, assuming that if the user activated populating the fields she also wants to include them in the report, which is a reasonable assumption.

TestSuiteXmlParser no longer ignores the 'system-out' and 'system-err'
tags from the Surefire report XML. It parses them and sets the
corresponding properties in ReportTestCase.
@kriegaex
Copy link
Contributor Author

kriegaex commented Dec 6, 2023

@michael-o, FYI: I rebased on the latest changes, resolving a merge conflict, and also used mvn spotless:apply to satisfy the formatting build requirements.

@michael-o
Copy link
Member

@kriegaex thanks, will try to review asap. Corona has knocked me out.

@kriegaex
Copy link
Contributor Author

kriegaex commented Dec 7, 2023

thanks, will try to review asap. Corona has knocked me out.

@michael-o, I wish you a swift recovery. Take good care of yourself.

@acouvreur
Copy link

Thanks for this feature @kriegaex !
I would also need this feature, do you need any help on getting the work done ?
I'm able to help

@michael-o
Copy link
Member

Thanks for this feature @kriegaex ! I would also need this feature, do you need any help on getting the work done ? I'm able to help

Here is a precursor: #670

@kriegaex
Copy link
Contributor Author

kriegaex commented Mar 9, 2024

@acouvreur, actually I am just the user having opened SUREFIRE-2219, not the developer implementing it. Back then, I just had 45 minutes to investigate and do some back-end leg work to hopefully expedite the implementation, handing this off to any committers who feel up for the task and are way more competent regarding content generation via the sink API than I will ever be. I am afraid, I might just create a mess and cleaning it up might take more effort than someone else doing it right.

case "system-err":
currentElement = new StringBuilder();
parseContent = true;
break;
Copy link
Member

Choose a reason for hiding this comment

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

This is incomplete/imprecise. Look at the schema: https://maven.apache.org/surefire/maven-surefire-plugin/xsd/surefire-test-report.xsd. Testcase can have stdout/stderr, but also reruns and flakes. We should basically overwrite in that case. We need a way to make sure that these are direct descendants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My PR ist half a year old. I created it, because you asked me to. It is meant to be something you can commit on top of and refine. "Allow edits by maintainers" is active for this PR. Please do so.

Currently, my personal situation does not permit me to spend time on it, because on top of being busy with work I am in the process of relocating to another continent and was also thrown back by a recent stay in hospital after an accident, which cost me extra time. This is not an excuse, just an explanation. I am still interested in getting this issue resolved, only the time window to further collaborate on it has closed for now.

@kriegaex kriegaex marked this pull request as ready for review June 30, 2024 08:26
@kriegaex
Copy link
Contributor Author

FYI, I switched this draft PR to "ready for review", so it can be merged after someone will have committed review findings on top of it.

@michael-o
Copy link
Member

Sure, don't worry. Just wanted to point out that this is incomplete.

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.

3 participants