Implement branch coverage support for exit status modifiers #934

jemmaissroff · 2020-10-07T19:51:47Z

This implements branch coverage support for :minimum_coverage_by_file, :maximum_coverage_drop, and :refuse_coverage_drop.

Resolves Issue #844.

jemmaissroff · 2020-10-07T21:09:26Z

@PragTob this is my first PR in this repo! I tried to follow the contributions guide, but please, please let me know if there's anything I missed

jemmaissroff · 2020-10-07T21:10:13Z

lib/simplecov/configuration.rb

    end

    #
    # Refuses any coverage drop. That is, coverage is only allowed to increase.
    # SimpleCov will return non-zero if the coverage decreases.
    #
-    def refuse_coverage_drop
-      maximum_coverage_drop 0
+    def refuse_coverage_drop(criteria = [DEFAULT_COVERAGE_CRITERION])


Is this desired default behavior? Or would desired default behavior be to refuse coverage for lines and branches?

Great question!

I'd say default should be for both. It's backwards incompatible but it's the right behavior imo.

jemmaissroff · 2020-10-22T20:46:38Z

Hi @PragTob, just wanted to re-ping this PR in case it was missed :) Let me know how it looks!

PragTob

Thank you for the great PR! 💚 🎉 🚀

As already discussed elsewhere, terribly sorry for the long time it took me to get to this. Had a rough time and spent no time on OSS minus one bug fix release. It's not the standard I hold myself to, esp. for a new contributor and such extensive work. So, sincerest apologies again.

Also, yes there are a lot of comments - but it's a great PR. I review very thoroughly and a lot of the comments are just naming discussions or ideas how to cut maybe a bit of code. Some of them also require no action at all and are just little thoughts. Also just because I comment something doesn't mean I'm right - I can be wrong so please counter argue and ask 😁

Some things that we still need to do here:

make sure it works when a last_run file in the old format is present so people upgrading aren't greeted by an error, this would probably be best done through a cucumber scenario

check that these work in general also as a feature test (SimpleCov breaks in unexpected ways and is a big integration nightmare hence I like good integration tests that for instance check that these actually make a build fail). We do something similar in the miniminimum coverage feature:

simplecov/features/minimum_coverage.feature

Lines 65 to 81 in aa64b87

    
             @branch_coverage 
        
             Scenario: Works together with branch coverage and the new criterion announcing both failures 
        
               Given SimpleCov for Test/Unit is configured with: 
        
                 """ 
        
                 require 'simplecov' 
        
                 SimpleCov.start do 
        
                   add_filter 'test.rb' 
        
                   enable_coverage :branch 
        
                   minimum_coverage line: 90, branch: 80 
        
                 end 
        
                 """ 
        
               When I run `bundle exec rake test` 
        
               Then the exit status should not be 0 
        
               And the output should contain "Line coverage (88.09%) is below the expected minimum coverage (90.00%)." 
        
               And the output should contain "Branch coverage (50.00%) is below the expected minimum coverage (80.00%)." 
        
               And the output should contain "SimpleCov failed with exit 2"

unit testing the refuse_coverage_drop new way of specifying the criteria + fixing it

I know lots of this is cucumber related which quite some people are not familiar with. I'm happy to help you point you the right direction. If you feel like it'd be too much I'm also happy to take it over myself.

You made a lot of great work, I let you wait so long and I comment so much - have a couple of extra bunnies:

PragTob · 2020-11-29T18:46:13Z

CHANGELOG.md

+==========
+
+## Enhancements
+* Can now define the minimum_coverage_by_file, maximum_coverage_drop and refuse_coverage_drop by branch as well as line


"for branch as well as line coverage" might read nicer :)

PragTob · 2020-11-29T18:46:56Z

README.md

+# same as above (the default is to only refuse line drop)
+SimpleCov.refuse_coverage_drop :line
+# refuse drop for line and branch
+SimpleCov.refuse_coverage_drop :line, :branch


Thanks for updating the docs and with nice examples to boot! 👍

PragTob · 2020-11-29T18:50:15Z

features/maximum_coverage_drop.feature

@@ -40,7 +40,7 @@ Feature:
      """
      {
        "result": {
-          "covered_percent": 88.09
+          "line": 88.09


this is backwards incompatible, but I think it's fine. We should just mention it in the changelog. People might want to update their respective files to keep working after the release.

One thing though, we should write a test to make sure that we don't raise an error if an "old style" file is present. That'd be a catastrophic upgrade experience for folks if simplecov now failed all the time.

We could call it "covered_percent_branch" for branch coverage but it's not desirable long term, hence I'm fine with the incompatibility.

PragTob · 2020-11-29T18:51:11Z

lib/simplecov/configuration.rb

@@ -285,20 +285,22 @@ def merge_timeout(seconds = nil)
    #
    # Default is 0% (disabled)
    #
-
-    # rubocop:disable Metrics/CyclomaticComplexity


PragTob · 2020-11-29T18:55:16Z

lib/simplecov/configuration.rb

+      @minimum_coverage = coverage
+    end
+
+    def validate_coverage!(coverage, coverage_setting)


thanks for extracting!

Generally it's avoided to create bang/! methods if there's no non bang alternative. You probably did it to signify that it raises, we might extend the name to communicate that raise_on_invalid_coverage but just validate_coverage might also be fine.

I'll leave that naming decision up to you, but would like to get rid off the ! :)

PragTob · 2020-11-29T19:25:27Z

lib/simplecov/exit_codes/minimum_coverage_by_file_check.rb

+      def coverage_statistics_by_file
+        @coverage_statistics_by_file ||=
+          (res = result.coverage_statistics_by_file).each do |criteria, stats|
+            res[criteria] = stats.map { |stat| SimpleCov.round_coverage(stat.percent) }


Do we need this code?

I'm probably be wrong but we're "just" rounding the percentages, couldn't we do that down in compute_minimum_violations while we're mapping and call SimpleCov.round_coverage on actual_percent there? 🤔

PragTob · 2020-11-29T19:27:58Z

lib/simplecov/exit_codes/minimum_coverage_by_file_check.rb

+          end
+      end
+
+      def compute_minimum_violations


similar these aren't the violations yet so I'd name it something like compute_minimum_coverage_data or something as with drop above

PragTob · 2020-11-29T19:35:10Z

spec/configuration_spec.rb

@@ -47,70 +47,70 @@
      end
    end

-    describe "#minimum_coverage" do
+    shared_examples "checks coverage settings" do |coverage_setting|


I feel like "setting coverage expections" or something would be a better name (we also check the results not just that what is given is correct)

PragTob · 2020-11-29T19:36:48Z

spec/configuration_spec.rb

      after :each do
        config.clear_coverage_criteria
      end

      it "does not warn you about your usage" do
        expect(config).not_to receive(:warn)
-        config.minimum_coverage(100.00)
+        config.send(coverage_setting, 100.00)


hey can you please replace send with public_send basically everywhere here? :)

It's a pet peeve of mine, it helps us in only calling public methods so we won't accidentally make them private or something + highlighting we are indeed testing public methods and not private ones

Thanks for the tip, I didn't know about public_send!

spec/configuration_spec.rb

jemmaissroff

Thanks for the feedback @PragTob , round 1 of changes are up! It is very possible I missed one (or more!) of your comments. If so, please let me know 😄

jemmaissroff · 2020-11-30T15:12:48Z

spec/configuration_spec.rb

      after :each do
        config.clear_coverage_criteria
      end

      it "does not warn you about your usage" do
        expect(config).not_to receive(:warn)
-        config.minimum_coverage(100.00)
+        config.send(coverage_setting, 100.00)


Thanks for the tip, I didn't know about public_send!

PragTob

Looking good! Only minor comments I think. I have no idea why the CI didn't run though 🤷 😱

PragTob · 2020-12-19T10:58:35Z

lib/simplecov/configuration.rb

+    def refuse_coverage_drop(*criteria)
+      if criteria.empty?
+        SUPPORTED_COVERAGE_CRITERIA.each do |coverage_criteria|
+          enable_coverage(coverage_criteria)


I don't think we should enable all coverage if we're not given any arguments 🤔

We should probably just use all the coverage critieria that have been enabled. There should be a reader for that 🤔

PragTob · 2020-12-19T10:58:54Z

lib/simplecov/exit_codes/maximum_coverage_drop_check.rb

@@ -4,6 +4,7 @@ module SimpleCov
  module ExitCodes
    class MaximumCoverageDropCheck
      def initialize(result, maximum_coverage_drop)
+        puts "REPORTING*****"


debug statements :)

PragTob · 2020-12-19T11:08:57Z

lib/simplecov/exit_codes/maximum_coverage_drop_check.rb

+      def last_coverage(criterion)
+        last_run[:result][criterion].tap do |last_coverage_percent|
+          if !last_coverage_percent && criterion == "line"
+            return last_run[:result][:covered_percent]


I don't think this works 🤔

irb(main):001:0> true.tap do return false end Traceback (most recent call last): 6: from /home/tobi/.asdf/installs/ruby/2.7.2/bin/irb:23:in `<main>' 5: from /home/tobi/.asdf/installs/ruby/2.7.2/bin/irb:23:in `load' 4: from /home/tobi/.asdf/installs/ruby/2.7.2/lib/ruby/gems/2.7.0/gems/irb-1.2.6/exe/irb:11:in `<top (required)>' 3: from (irb):1 2: from (irb):1:in `tap' 1: from (irb):1:in `block in irb_binding'

As in can't return from inside the block and iirc tap just returns self anyhow. I think a "normal" if without tap should do the trick.

Also made me realize CI didn't run.. grmll.

jemmaissroff

@PragTob thanks for the review, up again! I'm also unsure why CI didn't run.... Maybe if I squash commits? Any suggestions there?

PragTob · 2020-12-21T13:54:42Z

@jemmaissroff I have no clue :( But it's most definitely on my/our side. Or on github actions. I did research it for 30 mins last time and hoped it was just a hiccup. I'll see if I can reproduce when doing my own fork and pushing to it. Might still be different but I'll give it a shot and try to fix it because without we can't really merge. (I mean ok technically I could put this on a main line branch and see it there but this setup should work).

Also super confusing since it did work on opening the PR.

PragTob · 2020-12-21T15:00:37Z

@jemmaissroff Ok I tried something I'm not sure if it'll work as my test didn't work, but that might be because it was my account. If this doesn't fix it I'll probably have to create a secondary account to test it out.

You might need to rebase the PR though (or merge new main) as the changes might only apply if your base reference is the new main of the repo.

PragTob

Looking good to me if we can convince CI to run. Thank you 💚

jemmaissroff · 2020-12-21T16:08:58Z

@PragTob okay, rebased off of main, and CI ran and passed 🎉 ! So should be all set to merge? Let me know if I need to squash my commits or anything else?

PragTob · 2020-12-22T11:32:15Z

@jemmaissroff looks good thank you! I think I'll give it another pass and if it's nothing major I'll just do it myself (changes + squashing) - all good. Thank you so much!

PragTob · 2020-12-22T11:46:00Z

All good to go! 🚀 💚

jemmaissroff force-pushed the branch-support-exit-modifiers branch 2 times, most recently from 0605c7b to d61ae99 Compare October 7, 2020 20:05

jemmaissroff commented Oct 7, 2020

View reviewed changes

jemmaissroff mentioned this pull request Oct 12, 2020

100% code coverage panorama-ed/memo_wise#62

Merged

PragTob reviewed Nov 29, 2020

View reviewed changes

jemmaissroff commented Dec 15, 2020

View reviewed changes

PragTob reviewed Dec 19, 2020

View reviewed changes

jemmaissroff commented Dec 20, 2020

View reviewed changes

PragTob mentioned this pull request Dec 21, 2020

Checking if CI works #953

Closed

PragTob approved these changes Dec 21, 2020

View reviewed changes

jemmaissroff added 3 commits December 21, 2020 10:54

Implement branch coverage support for exit status modifiers

6519bf1

Round 1 changes

63ef506

Round 2 changes

6722a7c

jemmaissroff force-pushed the branch-support-exit-modifiers branch from d245717 to 6722a7c Compare December 21, 2020 15:55

PragTob merged commit 542cd72 into simplecov-ruby:main Dec 22, 2020

PragTob mentioned this pull request Dec 22, 2020

Fix tests #956

Merged

f-moya mentioned this pull request Nov 27, 2021

Implement branch coverage support for exit status modifiers #844

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement branch coverage support for exit status modifiers #934

Implement branch coverage support for exit status modifiers #934

jemmaissroff commented Oct 7, 2020

jemmaissroff commented Oct 7, 2020

jemmaissroff Oct 7, 2020

PragTob Nov 29, 2020

jemmaissroff commented Oct 22, 2020

PragTob left a comment

PragTob Nov 29, 2020

PragTob Nov 29, 2020

PragTob Nov 29, 2020

PragTob Nov 29, 2020

PragTob Nov 29, 2020

PragTob Nov 29, 2020

PragTob Nov 29, 2020

PragTob Nov 29, 2020

PragTob Nov 29, 2020

jemmaissroff Nov 30, 2020

jemmaissroff left a comment

jemmaissroff Nov 30, 2020

PragTob left a comment

PragTob Dec 19, 2020

PragTob Dec 19, 2020

PragTob Dec 19, 2020

jemmaissroff left a comment

PragTob commented Dec 21, 2020

PragTob commented Dec 21, 2020

PragTob left a comment

jemmaissroff commented Dec 21, 2020

PragTob commented Dec 22, 2020

PragTob commented Dec 22, 2020

	@branch_coverage
	Scenario: Works together with branch coverage and the new criterion announcing both failures
	Given SimpleCov for Test/Unit is configured with:
	"""
	require 'simplecov'
	SimpleCov.start do
	add_filter 'test.rb'
	enable_coverage :branch
	minimum_coverage line: 90, branch: 80
	end
	"""

	When I run `bundle exec rake test`
	Then the exit status should not be 0
	And the output should contain "Line coverage (88.09%) is below the expected minimum coverage (90.00%)."
	And the output should contain "Branch coverage (50.00%) is below the expected minimum coverage (80.00%)."
	And the output should contain "SimpleCov failed with exit 2"

Implement branch coverage support for exit status modifiers #934

Implement branch coverage support for exit status modifiers #934

Conversation

jemmaissroff commented Oct 7, 2020

jemmaissroff commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jemmaissroff commented Oct 22, 2020

PragTob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jemmaissroff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragTob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jemmaissroff left a comment

Choose a reason for hiding this comment

PragTob commented Dec 21, 2020

PragTob commented Dec 21, 2020

PragTob left a comment

Choose a reason for hiding this comment

jemmaissroff commented Dec 21, 2020

PragTob commented Dec 22, 2020

PragTob commented Dec 22, 2020