Refactor RSpec queue runner

[tubaxenor]

Currently rspec queue runner calls RSpec::Core::Runner#run multiple times for each batch and have to reset runner multiple times. This PR makes following changes:

• Delegate calls to RSpec::Core::Runner instance and redo run_specs to wrap all batches from test file allocator
• Remove accumulator concept and replaced with knapsack_pro_batches
• Retain subset queue hook behavior and remove KnapsackPro::Hooks::Queue.call_before_queue and KnapsackPro::Hooks::Queue.call_after_queue, since they should be covered by RSpec suite hooks.

[ArturT]

@tubaxenor Thank you for the PR. It looks great.

I added an internal ticket to our backlog to review this, test it and update specs.

I also enabled CircleCI builds for forked pull requests so we could run tests on this PR.

Is it fine if we would like to update something (code, specs, docs) in this PR? I see the option Maintainers are allowed to edit this pull request. is enabled.

Could you run knapsack_pro based on this fork on your CI for a week or two to ensure there are no edge cases or nothing unexpected happens?

Does it help to reduce memory pressure caused by RSpec on CI machines? I suspect memory pressure was bigger when using the RSpec split by test examples feature, how does it look now?

[tubaxenor]

Hi Artur, I am still running some experiment builds and will get back to you once there are some data comparison to shared.

You are more than welcome to make changes to this PR since I probably don't have much context of the spec setup and doc convention etc. I will still make some code changes mostly for the logging part to make the code cleaner and more readable.

[ngan]

Could you run knapsack_pro based on this fork on your CI for a week or two to ensure there are no edge cases or nothing unexpected happens?

We will be using this runner internally and report back.

Does it help to reduce memory pressure caused by RSpec on CI machines?

This is something that'll be hard to measure, but we'll try. Besides this, there are several other benefits:

• Having before/after :suite callbacks work as expected (1 call each) is nice. No more custom Knapsack Pro callback.
• There's only 1 copy of the failures in the output as opposed to it being reprinted for each batch.
• Nice, clean green dots for a single test node.

As for the batch printouts, we can still let that happen between the green dots. We personally don't like it or use it, so we'd change the logging level to WARN to get rid of them. You could also log those batch information to files (which we do).

[ArturT]

I like the benefits you listed.

Keep us informed after you run this on your CI for some time. It would be nice if you could run this for your whole team. You could use the fork conditionally to have the option to quickly disable it for all feature branches in case you discover an unexpected issue.

For example, I use env var to decide if I want to use the knapsack_pro gem from the RubyGems or local copy:
https://github.com/KnapsackPro/rails-app-with-knapsack_pro/blob/a5d9cbddb6f43ab0c33adf2399d7c680a0fa0e3a/Gemfile#L62

[technicalpickles]

Currently rspec queue runner calls RSpec::Core::Runner#run multiple times for each batch and have to reset runner multiple times.

I did find another benefit to doing this. If you have config.example_status_persistence_file_path set, you end up loading and dumping the status for all specs each time.

We ended up disabling this during CI. I could see it being a knapsack pro default if there is a good place to set it. (can create a separate issue for it).

[ArturT]

@ngan @tubaxenor Have you had a chance to measure the memory pressure reduction in your project case?

I'm curious if you see memory usage reduced by a few %, or maybe something more drastic like 10-50%, which would be a huge win. Thanks.

[ngan]

@ArturT we ended not measuring memory usage because it was too difficult to do with our test suite size...and we felt that the other benefits only was worth making this change.

But we've been using this code for 2 months now and it's been working well.

[ArturT]

@ngan Thanks for the info. We have this PR in our backlog to review it and verify its approach. We also need to sync it with our latest changes in the knapsack_pro gem 6.x around RSpec integration. We will keep you posted with further updates.

[ArturT]

@tubaxenor I tested your code to see if it can also help with the issue:

• Do not allow to use the RSpec tag option together with the RSpec split by test examples feature #139 (comment)

I noticed that RSpec ignores the --tag option provided to the Knapsack Pro command. The --format d or --format p works fine.

Example command: bundle exec rake "knapsack_pro:queue:rspec[--format d --tag mytag]"

Your code breaks the expected RSpec behavior because the --tag is ignored.

[ArturT]

I fixed the issue. The --tag option is respected by the RSpec now (commit - I'm doing refactor on a separate branch)

[ArturT]

We have released version 7.0.0 of the knapsack_pro gem. This version incorporates ideas from this pull request, among other enhancements.

Please find the migration steps at the following link:
https://github.com/KnapsackPro/knapsack_pro-ruby/blob/master/CHANGELOG.md#700