Skip to content

feat: return warnings in parsing info #79

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

Merged
merged 3 commits into from
May 16, 2025
Merged

Conversation

joseph-sentry
Copy link
Contributor

we want to be able to return warnings when parsing a file, this is for cases where we want to let the user know something's wrong but continue parsing the rest of the JUnit XML because it may be partially valid

this is being implemented because I realized that failing to parse the entire file because one test case might have a really long name is not ideal for users, but we shouldn't silently skip including that test case either, we still want to let them know that something's in their JUnit XML isn't being parsed

this commit also includes changes to the attribute parsing so we can error when we fail to parse attributes, and return warnings when we fail to validate the attribute's value (see AttrsOrWarning)

this also means that i added logic for signalling that we're ignoring the test case we're currently parsing by making saved_testrun an enum (see TestrunOrSkipped)

I'm not sure if making warnings an Option<Vec> would be better than making it a Vec and having it be empty

also the fact that we have to pass WarningInfo objects back from the use_reader function then format it to include location info is unfortunate but it lets us avoid iterating over the buffer to get the line/col every time we want to create a warning

also the decision to include the warnings in the parsing info is to avoid breaking the interface of the parse_raw_upload function, which still returns the same tuple

@codecov-staging
Copy link

codecov-staging bot commented May 9, 2025

Codecov Report

Attention: Patch coverage is 85.35354% with 29 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/junit.rs 81.57% 28 Missing ⚠️
src/raw_upload.rs 97.67% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link

codecov bot commented May 9, 2025

Codecov Report

Attention: Patch coverage is 85.35354% with 29 lines in your changes missing coverage. Please review.

Project coverage is 95.78%. Comparing base (14fe3d7) to head (f129010).
Report is 4 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/junit.rs 81.57% 28 Missing ⚠️
src/raw_upload.rs 97.67% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #79      +/-   ##
==========================================
- Coverage   96.84%   95.78%   -1.07%     
==========================================
  Files          16       17       +1     
  Lines        1934     2038     +104     
==========================================
+ Hits         1873     1952      +79     
- Misses         61       86      +25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

codspeed-hq bot commented May 9, 2025

CodSpeed Performance Report

Merging #79 will not alter performance

Comparing joseph/warnings (f129010) with main (14fe3d7)

Summary

✅ 6 untouched benchmarks

@joseph-sentry joseph-sentry requested a review from a team May 9, 2025 19:09
we want to be able to return warnings when parsing a file, this is for
cases where we want to let the user know something's wrong but
continue parsing the rest of the JUnit XML because it may be partially
valid

this is being implemented because I realized that failing to parse the
entire file because one test case might have a really long name is not
ideal for users, but we shouldn't silently skip including that test
case either, we still want to let them know that something's in their
JUnit XML isn't being parsed

this commit also includes changes to the attribute parsing so we can
error when we fail to parse attributes, and return warnings when we
fail to validate the attribute's value (see AttrsOrWarning)

this also means that i added logic for signalling that we're ignoring
the test case we're currently parsing by making saved_testrun an enum
(see TestrunOrSkipped)

I'm not sure if making warnings an Option<Vec<String>> would be better
than making it a Vec<String> and having it be empty

also the fact that we have to pass WarningInfo objects back from the
use_reader function then format it to include location info is
unfortunate but it lets us avoid iterating over the buffer to get the
line/col every time we want to create a warning

also the decision to include the warnings in the parsing info is to
avoid breaking the interface of the parse_raw_upload function, which
still returns the same tuple
src/junit.rs Outdated
fn get_relevant_attrs(attributes: Attributes) -> PyResult<RelevantAttrs> {
let mut rel_attrs = RelevantAttrs::default();
// originally from https://gist.github.com/scott-codecov/311c174ecc7de87f7d7c50371c6ef927#file-cobertura-rs-L18-L31
fn parse_testcase_attrs(attributes: Attributes) -> Result<AttrsOrWarning> {
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of having an enum for this, have we considered creating a specific Err type for "we're missing some info" and matching the error returned from the Result to that error type so we know some info is missing?

The caller of parse_testcase_attrs can infer if parsing attributes failed with a "recuperable" error. Would let you keep the nice interface of RelevantAttrs and maybe simplify the code a little bit

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, that would make things here a bit cleaner.


impl WarningInfo {
pub fn new(message: String, location: u64) -> Self {
Self { message, location }
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 location the byte inside the file? (just curious)

<testsuite name="Title" errors="0" failures="0" skipped="0" timestamp="2023-11-10T17:59:47"
time="0.3" tests="4">
<testcase
classname="Really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really really long class name"
Copy link
Contributor

Choose a reason for hiding this comment

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

😂😂😂 great test. Seems true.

Copy link
Contributor

Choose a reason for hiding this comment

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

lolol, glad you pointed this out

Copy link
Contributor

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

I think there might be things to improve here, but also I don’t want to block this for too long. So its fine to land as-is, and improve it later.

@@ -170,6 +170,7 @@ impl Testrun {
pub struct ParsingInfo {
pub framework: Option<Framework>,
pub testruns: Vec<Testrun>,
pub warnings: Vec<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would actually call this errors

src/junit.rs Outdated
fn get_relevant_attrs(attributes: Attributes) -> PyResult<RelevantAttrs> {
let mut rel_attrs = RelevantAttrs::default();
// originally from https://gist.github.com/scott-codecov/311c174ecc7de87f7d7c50371c6ef927#file-cobertura-rs-L18-L31
fn parse_testcase_attrs(attributes: Attributes) -> Result<AttrsOrWarning> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, that would make things here a bit cleaner.

parse_testcase_attrs returns a custom Result<ParseAttrsError> now
and the calling function is responsible for ignoring the Error that's
actually a warning
@joseph-sentry joseph-sentry merged commit f4f44a3 into main May 16, 2025
9 of 13 checks passed
@joseph-sentry joseph-sentry deleted the joseph/warnings branch May 16, 2025 15:11
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.

4 participants