-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: update test_results_parser version and start computing names #742
Conversation
This PR includes changes to |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #742 +/- ##
==========================================
- Coverage 98.03% 98.00% -0.03%
==========================================
Files 438 438
Lines 36469 36451 -18
==========================================
- Hits 35751 35725 -26
- Misses 718 726 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #742 +/- ##
==========================================
- Coverage 98.03% 98.00% -0.03%
==========================================
Files 438 438
Lines 36469 36451 -18
==========================================
- Hits 35751 35725 -26
- Misses 718 726 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #742 +/- ##
==========================================
- Coverage 98.03% 98.00% -0.03%
==========================================
Files 438 438
Lines 36469 36451 -18
==========================================
- Hits 35751 35725 -26
- Misses 718 726 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #742 +/- ##
==========================================
- Coverage 98.03% 98.00% -0.03%
==========================================
Files 438 438
Lines 36469 36451 -18
==========================================
- Hits 35751 35725 -26
- Misses 718 726 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This commit updates the version of the test_results_parser library that we are using: - no more support for parsing vitest.json or pytest-reportlog files - parse_junit_xml now returns a ParsingInfo class, the type signature looks something like: class ParsingInfo: testruns: list[Testrun] framework: framework where Framework is: class Framework(Enum): Pytest Jest Vitest PHPUnit - Testrun also now contains the filename field which is an optional string and a classname field We've also started computing the name of the test which just means depending on the framework we've detected we transform the raw name and classname from the JUnit file to something more readable and if that computed name is available it's what we should show in the PR comment
cf09fe4
to
1df0658
Compare
This PR includes changes to |
This PR includes changes to |
This PR includes changes to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments
tasks/test_results_processor.py
Outdated
print("BBBBBBBBBBBBBBBBBBBBBBBBBBBBB") | ||
except ParserError as e: | ||
print("AAAAAAAAAAAAAAAAAAAAAAAAAAAAA") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove print statements
@@ -0,0 +1 @@ | |||
{"network_files": ["api/temp/calculator/test_calculator.py"], "test_results_files": [{"filename": "codecov-demo/temp.junit.xml", "format": "base64+compressed", "data": "eJy1VMluwjAQvfMVI1dCoBbHZiklJEFVS4V66Kkqx8okBqw6i2KHwt/XWSChnCrRXDLjefNm8Uuc2T6UsOOpEnHkIooJAh75cSCijYsyve49oJnnaK60yoR5NWyIWMhdlBzyE5OWpnGqXGQY1kzILOXGoQjUl0gSHhSBItdFQ2OJPJdgMuqXjtIsTFzUJ/1Bj9IeuX+n1KZjmwwxoZSMDWwbK13W/HhZvC1fn5eLCZaxzyQq2/KZ4uBLplQJY4nAmocJNhA/k0zHKc5xn7WPqimKYxYEjc6Iad66DrHKVjplvv4f9jCTWiTy0GQnV2MPxE4E/Lxzz6muGMzFKbbJaZXiqQajIHBdIHjUvqFkCrcA31tugEUA2lJP11nkayM3eKobKIsA00D2lAz9CV9NSHujpx16B/3uievI9mceU/sChryAr6ExZKdrtwpw+VQjXeSVPVVjtubn6HoBp8iVlnDGd9VFtFpGFFYuCqsWgfVLFDg52ANiw2Mxpyk3zz94x6qU4DnWUW2VWfwkmrbyfgBbcXMH", "labels": ""}], "metadata": {}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can we format these json better? This will make it easier to read and debug errors.
tasks/test_results_processor.py
Outdated
# TODO: this is bad | ||
self.network = data.get("network_files") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add more context into why this is bad? I see that network_files
is supposed to be in the uploaded test file, but I don't have much context into why getting this here is a bad thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, this was a note for myself to revisit, the alternative to this is to return the network from the function, which i think makes more sense, but i wanted to quickly make sure it worked so i just set it as an attribute on the task itself, will fix this with the next commit
- change test_results_processor to pass network through to compute_name instead of setting it as an attribute on the Task - change sample_test files from txt to json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM! Just a question about one of the class names.
tasks/test_results_processor.py
Outdated
name = raw_name | ||
return name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simplification suggestion:
name = raw_name | |
return name | |
return raw_name |
test_instance_data = [] | ||
test_flag_bridge_data = [] | ||
daily_totals = dict() | ||
daily_totals: dict[str, dict[str, str | int | list[str]]] = dict() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not something to do in this PR, but I would consider creating a custom object/type/dataclass to simplify this complex type annotation.
tasks/test_results_processor.py
Outdated
|
||
|
||
@dataclass | ||
class argProcessingResult: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be ProcessingResult
or ArgProcessingResult
?
This commit updates the version of the test_results_parser library
that we are using:
no more support for parsing vitest.json or pytest-reportlog files
parse_junit_xml now returns a ParsingInfo class, the type signature
looks something like:
class ParsingInfo:
testruns: list[Testrun]
framework: framework
where Framework is:
class Framework(Enum):
Pytest
Jest
Vitest
PHPUnit
Testrun also now contains the filename field which is an optional
string and a classname field
We've also started computing the name of the test which just means
depending on the framework we've detected we transform the raw name
and classname from the JUnit file to something more readable and if
that computed name is available it's what we should show in the PR
comment