Skip to content
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.

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

Fixes #624 Checks max_coverage_drop on failed tests #625

Conversation

thomas07vt
Copy link
Contributor

@thomas07vt thomas07vt commented Oct 1, 2017

Fixes #624

Just taking out the max coverage drop check outside the success if statement. I can't think of a reason to leave it in the success if statement, but of course I am not too familiar with this codebase :) . Can you think of a reason why we would want to write to .last_run.json on failing tests?

The only thing that I wasn't sure of is, if a test is failing, maybe we want to leave the pre-existing non-zero exit status rather than writing an exit status of 3.

@PragTob
Copy link
Collaborator

PragTob commented Oct 4, 2017

Hi, first thanks for your contribution! I'll take a look, but first it seems that the build is failing because rubocop inspects some vendored gems... grml.

@thomas07vt
Copy link
Contributor Author

Yeah I checked the .rubocop.yml file and its using an exclude path that seems like it should work 'vendor/bundle/**/*' so I am not sure what's going on...

@PragTob
Copy link
Collaborator

PragTob commented Oct 4, 2017

and that failure is a bug in rubocop... PRing then workaround fixing. Then reviewing here hopefully :)

Copy link
Collaborator

@PragTob PragTob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋

So, thanks for the PR.

I'm unsure about changing the behavior to always check for coverage drop, especially when tests fail etc. we shouldn't do it imo.

So, imo we should probably just write the last_run json when it was successful - i.e. do the write within the bigger if. If I understand the bug correctly this should also solve it, right? :)

coverage_diff = last_run["result"]["covered_percent"] - covered_percent
if coverage_diff > SimpleCov.maximum_coverage_drop # rubocop:disable Metrics/BlockNesting
$stderr.printf("Coverage has dropped by %.2f%% since the last time (maximum allowed: %.2f%%).\n", coverage_diff, SimpleCov.maximum_coverage_drop)
@exit_status = SimpleCov::ExitCodes::MAXIMUM_COVERAGE_DROP
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh... I guess all that at_exit behaviour needs to find its way into some other place and refactored, not here, I'm even scared to touch it :D

@thomas07vt thomas07vt closed this Oct 4, 2017
@thomas07vt thomas07vt reopened this Oct 4, 2017
@PragTob
Copy link
Collaborator

PragTob commented Oct 4, 2017

Nope, this one we sitll need

@thomas07vt
Copy link
Contributor Author

thomas07vt commented Oct 4, 2017

If I understand the bug correctly this should also solve it, right? :)

yeah that should work too. I can update my PR to add that change.

@PragTob
Copy link
Collaborator

PragTob commented Oct 4, 2017

That'd be great!

@thomas07vt thomas07vt force-pushed the issues/624/refuse_coverage_drop_on_failed_test branch 3 times, most recently from b7615b3 to 1141ca3 Compare October 5, 2017 02:36
@thomas07vt
Copy link
Contributor Author

@PragTob would you prefer the duplicate write call as I have it in the PR, or a duplicate check on ```@exit_status`` like this:

    if @exit_status == SimpleCov::ExitCodes::SUCCESS # No other errors
      if covered_percent < SimpleCov.minimum_coverage # rubocop:disable Metrics/BlockNesting
        $stderr.printf("Coverage (%.2f%%) is below the expected minimum coverage (%.2f%%).\n", covered_percent, SimpleCov.minimum_coverage)
        @exit_status = SimpleCov::ExitCodes::MINIMUM_COVERAGE
      elsif covered_percentages.any? { |p| p < SimpleCov.minimum_coverage_by_file } # rubocop:disable Metrics/BlockNesting
        $stderr.printf("File (%s) is only (%.2f%%) covered. This is below the expected minimum coverage per file of (%.2f%%).\n", SimpleCov.result.least_covered_file, covered_percentages.min, SimpleCov.minimum_coverage_by_file)
        @exit_status = SimpleCov::ExitCodes::MINIMUM_COVERAGE
      elsif (last_run = SimpleCov::LastRun.read) # rubocop:disable Metrics/BlockNesting
        coverage_diff = last_run["result"]["covered_percent"] - covered_percent
        if coverage_diff > SimpleCov.maximum_coverage_drop # rubocop:disable Metrics/BlockNesting
          $stderr.printf("Coverage has dropped by %.2f%% since the last time (maximum allowed: %.2f%%).\n", coverage_diff, SimpleCov.maximum_coverage_drop)
          @exit_status = SimpleCov::ExitCodes::MAXIMUM_COVERAGE_DROP
        end
      end
    end

    if @exit_status == SimpleCov::ExitCodes::SUCCESS
      SimpleCov::LastRun.write(:result => {:covered_percent => covered_percent})
    end

I kinda like the duplicate @exit_status check better, but I'll defer to you :)

end
else
SimpleCov::LastRun.write(:result => {:covered_percent => covered_percent})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd just move the write tot he bottom of the bigger if and keep the exit_status check as before I think. But this whole code is ripe for some proper refactoring though :)

@thomas07vt thomas07vt force-pushed the issues/624/refuse_coverage_drop_on_failed_test branch from 1141ca3 to 66ffa82 Compare October 5, 2017 19:23
@thomas07vt
Copy link
Contributor Author

@PragTob How about this?

@PragTob
Copy link
Collaborator

PragTob commented Oct 6, 2017

Should be good enough, thanks! :)

This was referenced Mar 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants