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

feat: add collection of failure messages in Testruns #4

Merged
merged 2 commits into from
Jan 10, 2024

Conversation

joseph-sentry
Copy link
Contributor

No description provided.

Copy link

@adrian-codecov adrian-codecov left a comment

Choose a reason for hiding this comment

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

Looks good to me, although I'll let Matt take another look as still ramping up on Rust

src/junit.rs Outdated
@@ -45,6 +45,7 @@ fn populate(attr_hm: &HashMap<String, String>, testsuite: String) -> Result<Test
duration,
outcome: Outcome::Pass,
testsuite,
failure_message: s(""),

Choose a reason for hiding this comment

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

Do we want to specify an error?

Choose a reason for hiding this comment

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

i think instead failure_message should be Option<String> and it should be set to None here

Copy link

@matt-codecov matt-codecov left a comment

Choose a reason for hiding this comment

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

accept but pls look at feedback

src/junit.rs Outdated
@@ -45,6 +45,7 @@ fn populate(attr_hm: &HashMap<String, String>, testsuite: String) -> Result<Test
duration,
outcome: Outcome::Pass,
testsuite,
failure_message: s(""),

Choose a reason for hiding this comment

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

i think instead failure_message should be Option<String> and it should be set to None here

@@ -124,7 +125,18 @@ pub fn parse_junit_xml(file_bytes: Vec<u8>) -> PyResult<Vec<Testrun>> {
}
_ => (),
},
Ok(Event::Text(_)) => (),
Ok(Event::Text(x)) => {

Choose a reason for hiding this comment

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

will this always be a failure message? can we check anything about the context to make sure this is inside of a failure node?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've only seen examples of text being a failure message, but I'm not certain, so we could keep track of what kind of event this testcase contains (if it contains any kind of event)

src/junit.rs Outdated
xml_failure_message.inplace_trim_end();
xml_failure_message.inplace_trim_start();

testrun.failure_message = String::from_utf8(xml_failure_message.as_ref().to_vec())?;

Choose a reason for hiding this comment

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

i think you can do String::from_utf8(xml_failure_message.as_bytes()) and skip creating a vec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as_bytes does not seem to exist as a method on BytesText

@@ -20,6 +31,7 @@ struct PytestLine {
location: Option<Location>,
when: String,
outcome: String,
longrepr: Option<LongRepr>,

Choose a reason for hiding this comment

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

two things, which you can do in different PRs if you want:

  • can you please add doc comments (///) explaining what the structs/fields all are? no idea what ReprCrash and LongRepr are, no idea what values i can expect in when, outcome, report_type, not sure what start/stop are (probably timestamps...?)
  • when, outcome, report_type should probably be enums instead of strings. you can get serde to work with enums

- Make Testrun.failure_message Option type
- Use enums in Pytest-reportlog parser for when, outcome and report_type
- Check if in failure when handling text node in JUnit parser
@joseph-sentry joseph-sentry merged commit 5ac7608 into main Jan 10, 2024
@joseph-sentry joseph-sentry deleted the joseph/collect-failure-messages branch January 10, 2024 21:14
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.

3 participants