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

Show less context on failure #505

Merged
merged 1 commit into from
Apr 30, 2024
Merged

Conversation

sol
Copy link
Contributor

@sol sol commented Oct 22, 2023

Currently hedgehog error output is literally useless when used with hspec-hedgehog.

hedgehog includes the whole top-level definition in which the error occurred in the error output. Hspec test suites tend to define all tests in one top level definition. As a result, hspec-hedgehog will include the whole test suite in the error output for each failing test.

This produces lots of boring output with only little interesting output in between.

This PR allows the user to specify an upper limit of boring output lines that are printed around interesting output lines. This can be used by hspec-hedgehog to provide more useful error output.

hspec/hspec-hedgehog#35

@sol sol changed the title Only print a specified number of context lines in failure reports Show less context on failure Oct 22, 2023
@sol sol force-pushed the report-context branch 3 times, most recently from 0f8a9af to 895d1de Compare April 3, 2024 04:37
@sol sol marked this pull request as ready for review April 3, 2024 05:29
Copy link
Member

@moodmosaic moodmosaic left a comment

Choose a reason for hiding this comment

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

This looks good, and we could include it in the upcoming hedgehog 1.5 release 👍 🚀

I know it's a bit much to ask, but in respecting Jacob's memory, who was always using let instead of where, could we align with our style guidelines and make this adjustment? It’s a small tribute to his preferences and helps keep hedgehog codebase consistent too.

If you're not inclined to make this change, that's totally fine—I can take care of it before merging.

Thanks for your contribution, @sol!

@sol
Copy link
Contributor Author

sol commented Apr 9, 2024

@moodmosaic thanks a lot for taking a look at this.

could we align with our style guidelines

That makes sense, yes 👍

I can take care of it before merging.

If it's not too much trouble for you then that would be great.

@sol
Copy link
Contributor Author

sol commented Apr 30, 2024

I'm looking into it right now.

@sol sol marked this pull request as draft April 30, 2024 06:38
@sol sol marked this pull request as ready for review April 30, 2024 06:41
@sol sol requested a review from moodmosaic April 30, 2024 06:44
Copy link
Member

@moodmosaic moodmosaic left a comment

Choose a reason for hiding this comment

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

LGTM!

@moodmosaic moodmosaic merged commit c89ffd3 into hedgehogqa:master Apr 30, 2024
25 checks passed
@moodmosaic
Copy link
Member

@sol, thank you 👍

In order to continue with hspec/hspec-hedgehog#35 do we want a new release on hackage?

@sol
Copy link
Contributor Author

sol commented May 1, 2024

I'm gone use the git version for the time being. But once we have a release, all users will benefit from it, which I think will be a big improvement.

@moodmosaic
Copy link
Member

Of course 👍 Let’s plan for a release in May.

@sol sol deleted the report-context branch May 2, 2024 11:58
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.

None yet

2 participants