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.

Sign up for GitHub

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

Jump to bottom

Allow chained comparisons #21

Open

povilasjurcys wants to merge 1 commit into piotrmurach:master from povilasjurcys:allow_chained_comparisons

povilasjurcys commented Apr 26, 2023 •

edited

Loading

Describe the change

This PR allows setting at_most(x).at_least(x) without overriding previously defined expectations.

before the change:

expect { 2.times.map(&:to_s) }
  .to be_faster_then { 20.times.map(&:to_s) } # ratio is 10.0
  .at_least(99).times
  .at_most(100).times # PASSES because we check only `at_most` value

after the change

expect { 2.times.map(&:to_s) }
  .to be_faster_then { 20.times.map(&:to_s) } # ratio is 10.0
  .at_least(99).times
  .at_most(100).times # FAILS because we check `at_least` and  `at_most` values

Why are we doing this?

When testing performance with a comparison matcher it's almost impossible to set an exact value - each machine will perform a bit differently. But we can set a range where we expect the comparison to fall. Sadly it's hard to do so in an intuitive way as it's not possible to set minimum and maximum count for a single case. Having the possibility to set min and max times count would prevent from false-positives and allow us to better notice changes in performance,

Benefits

More intuitive expressions;
Lower risk of false-positives;

Drawbacks

Possible drawbacks applying this change: 🤷

Requirements


          Allow chained comparisons

d080b5f

houndci-bot reviewed

View reviewed changes

lib/rspec/benchmark/comparison_matcher.rb

                         if @comparison_type == :faster
-< @ratio && @ratio < @count
+< @ratio && @ratio < count

houndci-bot Apr 26, 2023

Style/YodaCondition: Reverse the order of the operands 1 < @ratio.

lib/rspec/benchmark/comparison_matcher.rb

+                      end
+                      def expected_count_matchers
+                        @expected_count_matchers.empty? ? DEFAULT_EXPECTED_COUNT_MATCHERS : @expected_count_matchers

houndci-bot Apr 26, 2023

Metrics/LineLength: Line is too long. [102/80]

lib/rspec/benchmark/comparison_matcher.rb

+                        expected_count_matchers
+                          .select { |_, count| count != 1 }
+                          .map { |type, count| "by #{type} #{count} times" }
+                          .join(' and ')

houndci-bot Apr 26, 2023

Layout/DotPosition: Place the . on the previous line, together with the method call receiver.
Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

lib/rspec/benchmark/comparison_matcher.rb

+                      def comparison_description
+                        expected_count_matchers
+                          .select { |_, count| count != 1 }
+                          .map { |type, count| "by #{type} #{count} times" }

houndci-bot Apr 26, 2023

Layout/DotPosition: Place the . on the previous line, together with the method call receiver.

lib/rspec/benchmark/comparison_matcher.rb

@@ @@ -162,6 +156,13 @@ def failure_reason @@
                       private
+                      def comparison_description
+                        expected_count_matchers
+                          .select { |_, count| count != 1 }

houndci-bot Apr 26, 2023

Layout/DotPosition: Place the . on the previous line, together with the method call receiver.

spec/unit/comparison_matcher_spec.rb

+                    it "fails if the block performance ratio is lower then indicated by at_most" do
+                      expect {
+                        expect {

houndci-bot Apr 26, 2023

Style/BlockDelimiters: Avoid using {...} for multi-line blocks.

spec/unit/comparison_matcher_spec.rb

+                    end
+                    it "fails if the block performance ratio is lower then indicated by at_most" do
+                      expect {

houndci-bot Apr 26, 2023

Style/BlockDelimiters: Avoid using {...} for multi-line blocks.

spec/unit/comparison_matcher_spec.rb

+                      }.to raise_error(/expected given block to perform faster than comparison block by at_least \d+ times and by at_most \d+ times, but performed faster by \d+.\d+ times/)
+                    end
+                    it "fails if the block performance ratio is lower then indicated by at_most" do

houndci-bot Apr 26, 2023

Metrics/LineLength: Line is too long. [85/80]

spec/unit/comparison_matcher_spec.rb

+                        expect { 1 << 1 }
+                          .to perform_faster_than { 'x' * 10 * 1024 }
+                          .at_least(2).at_most(15).times
+                      }.to raise_error(/expected given block to perform faster than comparison block by at_least \d+ times and by at_most \d+ times, but performed faster by \d+.\d+ times/)

houndci-bot Apr 26, 2023

Metrics/LineLength: Line is too long. [174/80]

spec/unit/comparison_matcher_spec.rb

+                      expect {
+                        expect { 1 << 1 }
+                          .to perform_faster_than { 'x' * 10 * 1024 }
+                          .at_least(2).at_most(15).times

houndci-bot Apr 26, 2023

Layout/DotPosition: Place the . on the previous line, together with the method call receiver.

Author

povilasjurcys commented Apr 27, 2023

@piotrmurach , not sure what's the proper way to deal with rubocop issues. Seems like the other code in the file is written ignoring them so I tried to keep the code consistent

Author

povilasjurcys commented Oct 4, 2023

@piotrmurach, what should I do in order to merge this PR?

pboling commented Mar 5, 2024

This is awesome! Ping @piotrmurach

BhagawatAdhikari approved these changes

View reviewed changes

pboling commented Jul 29, 2024

@piotrmurach Please consider this!

Owner

piotrmurach commented Aug 10, 2024

Hi @povilasjurcys,

Thank your contribution to rspec-benchmark.

The drawbacks section is important to me. It tells me how thoroughly a feature has been considered before I review anything. There are always drawbacks, even considering increased complexity and maintenance. Further, to add to the considerations:

The current implementation will allow incompatible chaining like at_most(...).exactly(...).
The chaining is unlimited, so at_most(...).at_least(...).at_most(...) will work.
I wonder whether we want to allow setting an upper limit before lower, as in, at_most(...).at_least(...).
Potentially, a different matcher could work better like be_between, in_range, to_span or something similar.

Most importantly, is it true that we reduce the risk of false positives by allowing chaining? I mean, when we use, for example, the at_least expectation, we want to prevent regression. My intention wasn't to prevent improvements to the speed profile. A given function may get faster for various reasons such as Ruby update. Is it desirable to fail in that instance? I'd argue that setting both lower and upper limits makes it more brittle. I added at_most as the 'reverse' of the at_least matcher to set the margin for the asymptotic behaviour depending on the comparison applied.

@pboling I appreciate your enthusiasm for this feature. Emojis are fine, I appreciate the feedback. However, please bear in mind that my time is limited and my priorities in open source do not necessarily align with what others want at a given moment. I have a lot of gems to maintain - nearly 70. If you wonder how you can help push things forward. You can do so by reviewing the patch code and providing a critical evaluation of the approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet