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

Adds unit-test xml report generation task #212

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

Conversation

msheiny
Copy link

@msheiny msheiny commented Jan 9, 2025

What's Changed

  • Adds a invoke helper task to generate an xml report file (coverage.xml). This can be used by tooling (such as VSCode code coverage extensions) for ingestion.

Of note that the XML has to be modified to work correctly with tooling outside the container. I first tried to use the std-lib xml to do this but received a ruff CVE warning. I didn't want to pull in a new dependency here, so I did the hacky thing to just manipulate the file with string replacement. Open to feedback whether that was the right call or not.

Earlier commit i worked around a CVE in XML but after feedback decided it was acceptable for dev environments and went back to using the stdlib xml helpers.

Testing

Tested this logic in the floor-plan app while I was reviewing a PR and it worked fine.

@msheiny msheiny force-pushed the unittest_xml_task branch from 97f64cc to b066d49 Compare January 9, 2025 23:51
@msheiny msheiny marked this pull request as ready for review January 9, 2025 23:53
Copy link
Contributor

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

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

I like the idea!

@task(pre=[unittest])
def unittest_xml_coverage(context):
"""Produce an XML coverage report that can be ingested into other tools."""
produce_xml_cmd = "coverage xml --include '{{ cookiecutter.app_name }}/*' --omit '*/migrations/*,*/tests/*'"
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it would be preferable not to omit migrations (since we should be sure to test data migrations if possible) and tests (since it would flag cases of dead test code that never gets called, etc.)

Copy link
Author

Choose a reason for hiding this comment

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

Took in changes in latest push

cur_dir = os.path.dirname(__file__)
cov_xml_path = os.path.join(cur_dir, "coverage.xml")

# I was originally going to use ET lib but linter throws a CVE warning
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be fine to use ElementTree and just add an appropriate noqa comment, IMHO.

Copy link
Author

Choose a reason for hiding this comment

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

Okay latest commit i went back to using ET

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