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

[WIP]: Randomize spec execution #5578

Conversation

VegaFromLyra
Copy link
Contributor

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

So it can generate a seed for each test execution which can then
be used to by rspec --bisect to repro order dependent flaky specs.

This is one approach to identify the root cause for #5228

It appears this is failing because current_user is nil before this spec is
run.

Related Tickets & Documents

#5228

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

So it can generate a seed for each test execution which can then
be used to by `rspec --bisect` to repro order dependent flaky specs.

This is one approach to identify the root cause for forem#5228

It appears this is failing because `current_user` is `nil` before this spec is
run.
@rhymes
Copy link
Contributor

rhymes commented Jan 19, 2020

Thanks for taking on the exploration of why tests are failing!

I had indeed submit a PR to enable it in April 2019 - #2466 - and they kept it for a full one day :D

It was reverted the day after - #2486 - because all hell brooke loose and unfortunately was never revisited. The builds indeed started failing because there are some unfortunate sequencing issues somewhere.

Let me know how can I assist you in getting to the bottom of this because I'd very much like tests not to have dependencies on each other.

This issue was opened as a reminder #2483

I also just noticed we removed this particularly flaky spec #5579

@citizen428 also suggested flagging flaky tests with an attribute to keep track of them

@citizen428
Copy link
Contributor

Yup, the idea here was to tag flaky tests with a custom attribute, which makes it possible to exclude them from spec runs:

it "something", :flaky do
  ...
end

Run with:

$ rspec --tag ~flaky

This allows us to skip known flaky specs at times when it's really inconvenient to have them fail, but it also somewhat lowers the incentive to find a proper fix. In our current situation I still think it'd be a good way to get a list of known flaky specs that lives in the codebase itself and doesn't get tracked via GH issues.

@VegaFromLyra
Copy link
Contributor Author

Ah good to know @rhymes and @citizen428!

The background's super helpful, I should have guessed this was attempted before. I was particularly trying to get to the bottom of #5228 since it has failed in most of my PR's and that's insightful that there have been issues with signing users in the past since that seems to be the root cause for this spec as well where current_user is mysteriously nil when executed in a certain order leading to that exception being raised.

My only concern with tagging flaky specs and potentially skipping them is that a subset of these flaky specs aren't the ones that need fixing, it's likely some other specs that leaking context.

@VegaFromLyra
Copy link
Contributor Author

What I am attempting currently is to make use of rspec bisect to get to the bottom of this flaky spec.

And interestingly enough I ran into another issue where bisect was hanging when I tried running it locally with this test suite, but good news that particular issue has been fixed in rspec core!

My next step is to see how to get this rspec fix in and try to leverage bisect to get to the bottom of this or other flaky specs.

On that note, was there any attempt to use rspec bisect in the past?

@maestromac
Copy link
Contributor

Yeah, we've used bisect to solve some bug in the past before to target a reproducible bug/issue. It's much harder to use that with flaky spec though since you are hoping that a problematic commit will properly fail said spec.

@VegaFromLyra
Copy link
Contributor Author

Yea it's likely a heavy handed approach to isolate a fix for that particular spec. I was initially thinking that I could identify which seed a build fails for and then use bisect and that seed to reproduce it locally.

But after some more reading I think the fix I mentioned above for rspec-core might only come out in rspec-rails 4 which hasn't been released, unclear what the plan actually is. So don't think that's a viable path forward (in the short term) but will look through the referenced PR's/issues to see if I can find an alternative.

@mstruve
Copy link
Contributor

mstruve commented May 7, 2020

Hey @VegaFromLyra!

Thank you so much for this PR! Currently, we are in the process of going through and cleaning up some of our PR history to help us better prioritize what we need to be focusing on. Since this PR has not been active for a while and from the comments it sounds like you are going to work on a different approach I am going to close it for now.

Thanks so much for being a contributor! Ping us anytime if you have questions!

@mstruve mstruve closed this May 7, 2020
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

5 participants