Fixed rspec --bisect to sort ids_to_run as the original order

[makicamel]

In RSpec::Core::Bisect::ExampleMinimizer#abort_if_ordering_inconsistent, minimizer checks the execution order of the devided ids are the same as the original order. So we need to sort them as the original order before run.

For example,
When we have 4 examples: 1.rb, 2.rb, 3.rb, 4.rb and 3.rb is failure order-dependent with 2.rb.
We expect to minimizer reproduce command rspec 2.rb[1] 3.rb[1], but minimizer raises 'The example ordering is inconsistent.'
This is because the ids to run are in the order 4.rb 2.rb (diffrent from the original), so fixed to sort them in the same order as the original error.

[pirj]

sufficient specs to cover the order?

Do you think it’s possible to cover the 4.rb 2.rb case that you mention in the description in a spec that would be red before the fix?

We usually do it ourselves, but I couldn’t find your real name. May I kindly ask you to add a Changelog entry?

[makicamel]

Do you think it’s possible to cover the 4.rb 2.rb case that you mention in the description in a spec that would be red before the fix?

Ahh, I apologize the lack of explanation 🙏
This spec covers the 4.rb 2.rb case that I mentioned in the description. But that spec was green also before this fix because FakeBisectRunner#run has a tiny mistake to unnecessary sort ids. This is the reason I fixed FakeBisecrRunner#run in this PR.
(I'm not good at English, so sorry if I didn't catch your intention 🙏)

And I tried to add a changelog! If I should squash commits, I'll do :)

[pirj]

If we add the sort back accidentally, will any spec fail? Is it possible to write such a spec?

[makicamel]

I see.
Because FakeBisectRunner is a test helper, I think it would be difficult to write a spec using a test helper fail when the test helper has a bug.

For example, next spec fails when we add sort back accidentally.

    it do
      all_ids = %w[ 1.rb[1] 2.rb[1] f.rb[1] 3.rb[1] ]
      dependent_ids = %w[ 2.rb[1] ]
      failure_id = "f.rb[1]"
      patterns = [
        [all_ids, [failure_id]],
        [[failure_id], []],
        [all_ids & (all_ids.slice(all_ids.size/2 .. -1) + [failure_id]), []],
        [all_ids & (all_ids.slice(0 .. all_ids.size/2) + [failure_id]), [failure_id]],
        [all_ids & (all_ids.slice(all_ids.size/2/2 .. all_ids.size/2) + [failure_id]), [failure_id]]
      ]
      allow(RSpec::Core::Bisect::ExampleSetDescriptor).to receive(:new).and_call_original
      patterns.each { |pattern|
        expect(RSpec::Core::Bisect::ExampleSetDescriptor).to receive(:new).with(*pattern)
      }

      fake_runner = FakeBisectRunner.new(
        all_ids,
        [],
        { failure_id => dependent_ids }
      )
      minimizer = new_minimizer(fake_runner)
      minimizer.find_minimal_repro
      expect(minimizer.repro_command_for_currently_needed_ids).to eq("rspec 2.rb[1] f.rb[1]")
    end

This spec tests a test helper FakeBisectRunner's internal logic, so I'm not sure this spec should be added.

[pirj]

Alright. But we've changed how get_expected_failures_for? works, which is not a spec helper. Is it possible to safeguard those changes with a spec? This spec would also
Please don't hesitate to tell if I'm talking a complete nonsense here. I'm reasoning from a generic position - if a change was made to the production code, there should either be:

• a way to reproduce the original issue reliably, and manually test that it fails without the fix and passes with the fix
• a way to reliably cover the same in a form of a (preferably human-readable) spec, both to document what was fixed, and to prevent regressions

Is it possible? Does it make sense? Does it apply in this case?

[makicamel]

Sorry for my bad explanation 🙏

First, because spec helper (FakeBisectRunner) has a tiny bug and I fixed it removing unnecessary sort. And then, when run this spec with the code removed spec helper's bug, it fails.
Second, now we have a failing spec, I fixed get_expected_failures_for?, library code.

So, the bug in the spec helper was getting in the way, but I think the spec already has a way to reliably reproduce the problem. I would be grateful if you could let me know if this doesn't seem to match your intentions 🙏

For documentation purpose the following spec could be added, but it would overlap with this spec.

    it 'returns the reproducing command even if there are unrelated ids after the failure id' do
      fake_runner = FakeBisectRunner.new(
        %w[ 1.rb[1] 2.rb[1] 3.rb[1] 4.rb[1] ],
        [],
        { "3.rb[1]" => %w[ 2.rb[1] ] }
      )
      minimizer = new_minimizer(fake_runner)
      minimizer.find_minimal_repro
      expect(minimizer.repro_command_for_currently_needed_ids).to eq("rspec 2.rb[1] 3.rb[1]")
    end

[JonRowe]

We have integration specs for the bisect runners it would be good to add a check to that which would fail if this wasn't implemented

[makicamel]

Thank you! I added an integration spec. 6877eb8

[JonRowe]

Can you rebase this once #3097 is merged / or cherry-pick that commit, theres an issue with our legacy actions needing fixing.

[makicamel]

@JonRowe
Thank you for fixing CI! I rebased.

[JonRowe]

Thanks

[JonRowe]

Released in 3.13.1 apologies for the delay