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

Output rspec seed if used #15

Merged
merged 1 commit into from
Nov 3, 2022
Merged

Output rspec seed if used #15

merged 1 commit into from
Nov 3, 2022

Conversation

grobie
Copy link
Contributor

@grobie grobie commented May 30, 2022

Rspec supports running the specs in a random order. This helps in
surfacing any accidental dependencies and side effects in the test
suite. When debugging test failures caused by such side effects, it's
often necessary to reproduce the issue by running the specs in the same
order again. For that the seed value needs to be known and logged by the
formatter.

@StefSchenkelaars
Copy link
Contributor

@grobie Thanks for the PR. Code loods good but I'm doubting if this should be in this formatter. For me it feels that this is another formatter (or an option for that matter).

What about splitting this output to another class completely and load both of them? Does that make sense for you too? You can then even make separate versions: one which outputs it like this one and maybe also one that translates it into a github action annotation. Let me know what you think about those things.

If you don't like that, I'm also fine with merging this.

@grobie
Copy link
Contributor Author

grobie commented Oct 31, 2022

@StefSchenkelaars Apologies for the late response. How important is it to you to split this out?

The seed in the output has helped us multiple times already to re-run flaky tests in the same order. I expect every rspec user who enables random spec runs to have the same need, so splitting this into multiple formatters seems unnecessarily complex.

My order of preference would be: 1) you merge this 😄 2) I change it as requested 3) We leave the PR unmerged

Copy link
Contributor

@StefSchenkelaars StefSchenkelaars left a comment

Choose a reason for hiding this comment

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

@grobie yea, now that I think of it, it's fine to add it here. I have some nitpicking point on the test. Would be nice if you can have a look at that. Then I'll try to get it merged (I'm not sure if I have access now).

spec/rspec/github/formatter_spec.rb Outdated Show resolved Hide resolved
@StefSchenkelaars
Copy link
Contributor

@grobie I got my permissions for the repo. Rubocop is complaining about an empty line. Once that's fixed I'll merge it 👍

Rspec supports running the specs in a [random order][1]. This helps in
surfacing any accidental dependencies and side effects in the test
suite. When debugging test failures caused by such side effects, it's
often necessary to reproduce the issue by running the specs in the same
order again. For that the seed value needs to be known and logged by the
formatter.

[1]: https://relishapp.com/rspec/rspec-core/v/3-8/docs/configuration/set-the-order-and-or-seed
@grobie
Copy link
Contributor Author

grobie commented Nov 1, 2022

Thanks @StefSchenkelaars, I made the requested change. It would be very helpful if you could release a new version afterwards, so that we can remove the git dependency to install from my branch. Cheers!

@StefSchenkelaars
Copy link
Contributor

@LuukvH Since this is a PR from a fork, reviewdog does not have the permissions to push the results through the Checks API. Could you disable the requirement for rubocop for a merge? This is covered by the reviewdog result itself as well.

@LuukvH LuukvH merged commit ddd92d6 into Drieam:master Nov 3, 2022
@grobie grobie mentioned this pull request Nov 3, 2022
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