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 TestRun, TestRunGroup and JUnit parser classes #318

Closed
wants to merge 9 commits into from

Conversation

joseph-sentry
Copy link
Contributor

This PR:

  • Creates the Test and Testrun classes to have a common representation of testing results between all parsers
  • Creates JUnitXMLParser class to be able to parse junit.xml files

@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

Merging #318 (d2bb464) into main (775ba46) will decrease coverage by 0.02%.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##             main     #318      +/-   ##
==========================================
- Coverage   95.55%   95.53%   -0.02%     
==========================================
  Files          79       82       +3     
  Lines        2745     2800      +55     
==========================================
+ Hits         2623     2675      +52     
- Misses        122      125       +3     
Flag Coverage Δ
python3.10 95.65% <94.44%> (-0.03%) ⬇️
python3.11 95.64% <94.44%> (-0.03%) ⬇️
python3.8 95.65% <94.44%> (-0.03%) ⬇️
python3.9 95.65% <94.44%> (-0.03%) ⬇️
smart-labels 95.52% <94.44%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
codecov_cli/parsers/__init__.py 100.00% <ø> (ø)
setup.py 0.00% <ø> (ø)
codecov_cli/parsers/junit.py 95.23% <95.23%> (ø)
codecov_cli/parsers/base.py 93.93% <93.93%> (ø)

@joseph-sentry joseph-sentry marked this pull request as ready for review October 23, 2023 15:26
class Test(object):
def __init__(self, name: str, status: bool, duration: timedelta):
self.name = name
self.status = status
Copy link
Contributor

Choose a reason for hiding this comment

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

I think status should be an Enum rather than a bool.
Or change it to "passed" or "successful"

skipped: int,
total: int,
):
self.testcases = testcases or []
Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't specify a default value for test cases (e.g. testcases: List[Test] = None then it can't ever be None, right?

(if it can then the type hint should be Optional)


def _create_testcase(self, testcase_xml: etree.Element):
return Test(
f"{testcase_xml.get('classname')}.{testcase_xml.get('name')}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be transformed in the format that we use for labels?
That is path/to/test.py::TestClassIfAny::test_function_name.

It would be helpful to use this in an ATS setting to have the test names follow this format.

if processed is None or len(processed) == 0:
raise ParsingError("Error parsing XML file")

testsuites = [
Copy link
Contributor

Choose a reason for hiding this comment

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

You are calling this testsuites but the type name is TestRun. I think you should pick one and stick to it. Whatever makes the most sense in the context.

self.skipped = skipped
self.total = total

def to_str(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

The uploader uploads JSON data... maybe we should have the encoding/decoding be to JSON?

If this is for debug purposes (e.g. printing the class nicely into console) than I believe the right function to override is __repr__.

self.duration = duration

def __repr__(self):
return f"{self.name}:{self.status}:{self.duration.total_seconds()}::"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this to be used as the encoding/decoding or just debugging?
If just for debugging than it's very confusing.

If it's for the encoding/decoding I think it's ok to use JSON directly.
I understand that the payload will be larger, but we compress it before uploading. And making the data easier to understand and work with seems more relevant... I could be wrong, if you have data backing up the fact that unless we encode the data in this compact way the resulting payload will be huge.

@joseph-sentry joseph-sentry changed the title Add Testcase, Testsuite and JUnit parser classes Add TestRun, TestRunGroup and JUnit parser classes Oct 24, 2023
@joseph-sentry
Copy link
Contributor Author

joseph-sentry commented Oct 24, 2023

Can this be transformed in the format that we use for labels? That is path/to/test.py::TestClassIfAny::test_function_name.

It would be helpful to use this in an ATS setting to have the test names follow this format.

So after having done some research the answer to this question is not straightforward. It turns out that the pytest junitxml plugin can generate two different versions of junit report, "xunit1" and "xunit2".

The "xunit1" version includes a file with each testcase that is generated in the report which should enable this, but I have not implemented support for this in this PR.

The "xunit2" version for which you can find the schema here can support including the file in the testsuite xml element, but at the moment it doesn't look like the pytest plugin generates that tag, so it won't be easy to create an ATS formatted name in this case, we would have to try and match the classname + name to the label and translate it. This is made easier by the fact that the path to the file in included in the classname (see this). If we have access to the labels in the CLI we can do this translation locally, otherwise we will have to do it after the upload.

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