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

Add missing lines-covered and lines-valid attributes #17738

Merged
merged 8 commits into from
Sep 15, 2024

Conversation

x612skm
Copy link
Contributor

@x612skm x612skm commented Sep 5, 2024

This PR resolves an issue where the Cobertura XML report generated by MyPy was missing the lines-covered and lines-valid attributes in the element.

Changes made:

  • Added the lines-covered and lines-valid attributes to the element in the Cobertura XML report.
  • Updated the CoberturaReportSuite test suite to validate that these attributes are correctly included in the generated XML.

Fixes #17689

x612skm and others added 2 commits September 5, 2024 16:24
… XML

This commit addresses the issue where the generated Cobertura XML report
was missing the `lines-covered` and `lines-valid` attributes.
The implementation now ensures these attributes are correctly populated.
Updated the `CoberturaReportSuite` test suite to validate the presence of
these attributes in the generated XML.

This comment has been minimized.

x612skm and others added 4 commits September 5, 2024 17:15
… XML

This commit addresses the issue where the generated Cobertura XML report
was missing the `lines-covered` and `lines-valid` attributes.
The implementation now ensures these attributes are correctly populated.
Updated the `CoberturaReportSuite` test suite to validate the presence of
these attributes in the generated XML.
# Conflicts:
#	mypy/test/testreports.py

This comment has been minimized.

Comment on lines 57 to 69
with tempfile.TemporaryDirectory() as tempdir:
reports = Reports(data_dir=tempdir, report_dirs={"dummy_report": tempdir})

cobertura_reporter = CoberturaXmlReporter(reports, ".")
cobertura_reporter.root_package = cobertura_package
cobertura_reporter.on_finish()

# Convert the XML document to a string
xml_str = etree.tostring(cobertura_reporter.doc, pretty_print=True).decode("utf-8")

# Check that the required attributes are present
assert f'lines-covered="{cobertura_package.covered_lines}"' in xml_str
assert f'lines-valid="{cobertura_package.total_lines}"' in xml_str
Copy link

Choose a reason for hiding this comment

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

Looks like you're testing a separate thing here. It's usually best to follow the AAA principle — checking one single thing per test: https://automationpanda.com/2020/07/07/arrange-act-assert-a-pattern-for-writing-good-tests/.
Hence, it's best to add new test cases unless you change the aspects of the existing ones. Just don't pile on extra logic. The tests must remain simple.

It's probably not a good idea to check substrings in XML text. No need to check if Python's XML writer works. The goal is to check that the reported produces the object having proper XML attributes. It's easier to check it against the object representation.

OTOH, looking closer, I think that the only change to really needed is changing that expected XML string on line 40. That test already checks everything needed, but you changed the produced output, so this test will keep failing until you adjust the expected value to match that new output shape.

Copy link
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

Copy link

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

@hauntsaninja this should be good to review now

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks!

@hauntsaninja hauntsaninja merged commit 77919cf into python:master Sep 15, 2024
19 checks passed
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.

[good-first-issue] Incomplete Cobertura XML spec support
3 participants