Fix #233 conflicts -> Update rubocop

[benoittgt]

I'm trying to reduce the number of open PR on this repo.

This PR is a merge conflict free of #233.

[benoittgt]

We have 3 rubocop offences

lib/rspec/support/comparable_version.rb:13:7: C: Metrics/AbcSize: Assignment Branch Condition size for <=> is too high. [23.62/22]
      def <=>(other) ...
      ^^^^^^^^^^^^^^
lib/rspec/support/spec/in_sub_process.rb:11:9: C: Metrics/AbcSize: Assignment Branch Condition size for in_sub_process is too high. [27.46/22]
        def in_sub_process(prevent_warnings=true) ...
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/rspec/support/method_signature_verifier.rb:283:7: C: Metrics/PerceivedComplexity: Perceived complexity for with_expectation is too high. [10/9]
      def with_expectation(expectation) # rubocop:disable MethodLength ...
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
31 files inspected, 3 offenses detected

Do we want to change Rubocop Metrics/AbcSize to 28 and Metrics/PerceivedComplexityto 10 or do we want to fix those issues?

I'm trying to fix them at the moment.

[benoittgt]

Little bump. I can work on any changes if needed.

[JonRowe]

Some refactoring of those methods would be good but happy for the rules to be disabled for those methods temporarily, isn't the rubocop.yml file controlled from the rspec-dev repo though?

[benoittgt]

You are correct for the rubocop.yml. I will remove the change on it, to stay sync with rspec-dev and see which change needs to be made.

[JonRowe]

@benoittgt I'm quite happy for you to update rubocop.yml over in rspec-dev if required :) Thank you for tackling this!

[benoittgt]

I don't know if we still need to keep benchmarks #233 (comment)

[benoittgt]

I started fixing offenses. But I still have 171 ! rubocop auto-correct break few test.
I will work on fixing those issues. I probably disable few cops on file when it require too much work.

[benoittgt]

I will probably import only benchmarks and speed changes on a new PR. I tried to rebase but it was very painful.

[pirj]

No worries about RuboCop part @benoittgt, I'm planning to address them in rspec-support as well.

[benoittgt]

Do we still want this?

[JonRowe]

I think so if you want to spend the time updating it, a bunch of the old ruby stuff is now irrelevant, and some of the rules will have been improved anyway?

[benoittgt]

We should be good now.

[pirj]

I don't know if we still need to keep benchmarks

As long as we use them to justify our styling choices - probably yes.

We can give in and use what rubocop-performance tells us to, however some express concerns regarding its defaults.

[JonRowe]

The changes to the code look fine, but what relevance do the benchmarks have?

[benoittgt]

The changes to the code look fine, but what relevance do the benchmarks have?

Good question. I just looked at commit body benoittgt@6b27ac2

lib/rspec/support/version_checker.rb:8:9: C: Do not use parallel assignment.
        @library_name, @library_version = library_name, library_version
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

But I should have look deeper. The file no longer exist and also we do not check this rule

rspec-support/.rubocop_rspec_base.yml

Line 130 in 8ba857e

Enabled: false

So I am in favor of removing this benchmark first.

For the second benchmark. This is related to a rule we didn't choose yet. benoittgt@6c8f550

We have not decided how we are going to handle the symbol to proc vs
block implementations. This is because the block is faster on Ruby
1.8.7, REE, and JRuby but slower on Ruby 1.9.2 through 2.2.2.

[pirj]

I guess we can put

Style/SymbolProc:
  Enabled: false

to .rubocop_todo.yml just like we do in rspec-rails. There should be a reference to it from .rubocop.yml.

Symbol to proc makes sense, we can decide/enforce later when we drop Ruby < 2.3.

[JonRowe]

I'm happy to keep either benchmark, but a comment at the top explaining that they are there for checking the performance of rubocop/rbhint recommendations

[pirj]

rbhint

😱 🏃 🙈

[JonRowe]

I wouldn't add this to the build personally, whats your thoughts here

[benoittgt]

I think it take some times to gather benchmark result on each Ruby build. You not get easily info that the new Ruby version is for example slower that the previous one on one benchmark. This is more something that should be inserted in https://rubybench.org/ruby/ruby/releases

Also it will add some time to the build itself. I think we should try to have always a fast CI.

I am not convinced about adding it. 😊

I would prefer a benchmark for each gem (except rspec-rails) that assert rspec gem benchmarking performance using a tool like https://github.com/marketplace/actions/continuous-benchmark (it needs Ruby support).

[JonRowe]

It might be an idea to run once, but not for every build.

[benoittgt]

I tried two things:

• Did what Aaron did. Run the benchmark on the CI. But we do not have bare metal server like for https://rubybench.org/hardware. So we cannot have real benchmark. One build can go on a machine that is slower or impacted about installed libraries. I don't think benchmark on CI is a good idea: https://bheisler.github.io/post/benchmarking-in-the-cloud/
• I tried https://github.com/benchmark-driver/benchmark-driver with bundle exec benchmark-driver -vv --bundler benchmarks/symbol_to_proc_vs_block.rb --rvm '2.2.2;2.3.1;2.4.6;2.5.5;2.6.6;2.7.1' and:

require 'benchmark_driver'

Benchmark.driver do |x|
  x.prelude <<~RUBY
    RANGE = (1..100)

    def sym_to_proc_with_map
      RANGE.map(&:to_s)
    end

    def sym_to_proc_with_each
      RANGE.each(&:to_s)
    end

    def block_with_map
      RANGE.map { |i| i.to_s }
    end

    def block_with_each
      RANGE.each { |i| i.to_s }
    end
  RUBY

  size = 10_000
  range = (1..size)

  x.report 'Block map', %{ block_with_map }
  x.report 'Symbol#to_proc map', %{ sym_to_proc_with_map }
  x.report 'Block each', %{ block_with_each }
  x.report 'Symbol#to_proc each', %{ sym_to_proc_with_each }
end

But the gem looks pretty buggy at the moment. I was not able to have the output for all ruby with with this command. Also results are not properly compared at the end you have to do it by hand. Plus, it is working only after Ruby 2.2.

Here is a cleaned output https://gist.github.com/benoittgt/3464d476060dbbe1dce5e7c649a3587a

BTW I noticed that the part with benchmark-ips was not written in the initial benchmark by Aaron. So I rewrote it.

I think both benchmark should be removed. I already removed one. If we want to have benchmarks like this one (basic Ruby across Ruby version), we should rely on rubybench.

[benoittgt]

Benchmark is incorrect. It should be:

Suggested change

	x.report('Block ') { range.map { |i| i.to_s } }
	x.report('Block ') { block_with_map }

This is the same for all the others.