Add plus no html skip generate html report - #285
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…l-skip-generate-html-report
| f"Skipping HTML report generation." | ||
| ) | ||
| return | ||
| if not session.config.getoption("--plus-no-html"): |
There was a problem hiding this comment.
@LeoAzLiu we may have a problem here:
the current flow is
- Screenshots are captured into the configured temporary screenshot directory, usually screenshots/.
- Their paths are recorded in the JSON report.
- The HTML generator finds those screenshots and copies them into report_output/screenshots/
- The original temporary screenshots/ directory is deleted.
- The generated HTML references the copied files under report_output/screenshots/.
With this introduction, I feel what would happen is, we will end up skipping step 3 and then still perform step 4, which means that, the screenshots would not get copied to report_output but at the same time delete the original screenshots folder which can cause data loss. We need to be able to decouple this or read this flag at that delete step and handle in this PR or let the user know that screenshots will not be created when html generation is disabled, given at the moment, we are not reading the screenshot file path in the vscode
There was a problem hiding this comment.
Yes. I prefer to keep the screenshots to report_output. I can try to handle this in this PR. Or anther before v1.3.0 to keep this PR been clear.
And I want covert to html and xml as python scripts. So if user want html or xml report can be possible. The command may like:
> pytest-html-plus final_report.json final_report.htmlThis will be break change. I have no idea if it possible or necessary.
There was a problem hiding this comment.
@LeoAzLiu XML generation is already optional through the --generate-xml flag. When enabled, users are explicitly opting into its additional processing and storage cost, which should be negligible.
One of our design principles is to minimize the number of steps users need to configure and execute. Requiring a second command to convert the generated JSON would add another step without providing much benefit. Users who need XML can enable the existing flag, while others avoid its small processing and storage cost.
There was a problem hiding this comment.
Yes. I prefer to keep the screenshots to report_output.
Yes, keeping screenshots under report_output/screenshots/ is the preferred behavior. It keeps all generated artifacts together and ensures the JSON report does not reference files that are deleted during cleanup.
Please handle this in the current PR because the issue is introduced by the new --plus-no-html execution path. When HTML generation is skipped, the screenshots should still be copied to report_output/screenshots/ before the original screenshot directory is removed. Please also add a test covering this behavior.
|
|
||
| # HTML options | ||
| group.addoption( | ||
| "--plus-email", |
There was a problem hiding this comment.
Combining --plus-no-html with --plus-email silently disables email. Is this mentioned in the documentation? or do you suggest having validators in place for incompatible combinations?
There was a problem hiding this comment.
To balance the impact of two options --plus-email force generate html may be good choice. I will fix this and mention this in doc.
There was a problem hiding this comment.
I would still feel lets not force generate, this will again come at a cost in time. I would prefer to keep in the docs that, if they have no html, --plus email will not work. --plus email is essentially just for emailing the html report so if they use --no-html then they are making a concious decision that it wont be available for email
@LeoAzLiu can you please see if you can address this? |
This is false alarm. The coverage calculate the exception that have not been covered as my changes because i changed branch. |
2 participants
Close #281
I re-organize the options code to clear the usage. Please help to review.