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 result in EvaluationResult #1209

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

flyinfish
Copy link

Add option to write result and expectedOutput to file when writing Evaluation Report
Closes #1208

@flyinfish flyinfish requested a review from a team as a code owner January 9, 2025 07:46
assertThat(content).contains("- **tag1**: 100.0");
assertThat(content).contains("- **tag2**: 0.0");
assertThat(content).contains("## Details");
assertThat(content).contains("### Sample1: PASSED");
Copy link
Author

@flyinfish flyinfish Jan 9, 2025

Choose a reason for hiding this comment

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

@cescoffier my take on reporting results
as it's supposed to be markdown this should be the way to go?

differs from - Sample1: PASSED, we could consider to harmonize it
for now it is backwards compatible

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it can be improved.

Reporting had not been my priority when I did that work.

This comment has been minimized.

Copy link
Collaborator

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Thanks!

var actualEvaluations = report.evaluations().stream()
.map(e -> "%s[%s;%s=%s]".formatted(e.sample().name(), e.sample().expectedOutput(), e.result(), e.passed()))
.toList();
assertThat(actualEvaluations).containsExactlyInAnyOrder(
Copy link
Author

Choose a reason for hiding this comment

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

@cescoffier order is random for now (was a bit of luck for the old version to be successful)
should n't we order it? (separate change)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it would be good to have a stable order.

Copy link
Author

Choose a reason for hiding this comment

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

i'll do it then

This comment has been minimized.

@flyinfish
Copy link
Author

Thanks!

pleasure
@geoand anything i need to do in order that it builds and gets merged or do you take care of it

@geoand
Copy link
Collaborator

geoand commented Jan 9, 2025

Not related to this PR, but @jmartisk I would like you to have a look at the CI failure when you have a minute. Nevermind :)

Add option to write result and expectedOutput to file when writing Evaluation Report
Closes quarkiverse#1208
@geoand
Copy link
Collaborator

geoand commented Jan 9, 2025

I rebased and force pushed to main

Copy link

quarkus-bot bot commented Jan 9, 2025

Status for workflow Build (on pull request)

This is the status report for running Build (on pull request) on commit f74c1b4.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@geoand geoand merged commit b980304 into quarkiverse:main Jan 9, 2025
70 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.

Missing "response" in EvaluationResult
3 participants