search

grodowski / undercover

undercover warns about methods, classes and blocks that were changed without tests, to help you easily find untested code and reduce the number of bugs. It does so by analysing data from git diffs, code structure and SimpleCov coverage reports
https://undercover-ci.com
MIT License
718 stars 28 forks source link

Undercover fails with shallow repository + grafted commits #175

Open kuahyeow opened 2 years ago

kuahyeow commented 2 years ago

On GitLab CI, the default depth for the cloned git repository is 50 (for projects like https://gitlab.com/gitlab-org/gitlab/, we set it even lower to 20, for speed purposes).

This leads to errors like below 

$ cat scripts/undercoverage
#!/usr/bin/env bash

bundle exec undercover -c "${1:-$(git merge-base origin/master HEAD)}"

$ UNDERCOVERAGE_COMPARE="${CI_MERGE_REQUEST_TARGET_BRANCH_SHA:-$CI_MERGE_REQUEST_DIFF_BASE_SHA}"
$ echo $UNDERCOVERAGE_COMPARE
b0238c51c69da80150745d92be243dc244d947b9

$ scripts/undercoverage ${UNDERCOVERAGE_COMPARE}
bundler: failed to load command: undercover (/builds/gitlab-org/gitlab/vendor/ruby/2.7.0/bin/undercover)
/builds/gitlab-org/gitlab/vendor/ruby/2.7.0/gems/undercover-0.4.4/lib/undercover/changeset.rb:77:in `merge_base': object not found - no match for id (a0036d11ff015325fe97e86aa0278b3e01e13bb3) (Rugged::OdbError)
    from /builds/gitlab-org/gitlab/vendor/ruby/2.7.0/gems/undercover-0.4.4/lib/undercover/changeset.rb:77:in `compare_base_obj'
    from /builds/gitlab-org/gitlab/vendor/ruby/2.7.0/gems/undercover-0.4.4/lib/undercover/changeset.rb:70:in `full_diff'
    from /builds/gitlab-org/gitlab/vendor/ruby/2.7.0/gems/undercover-0.4.4/lib/undercover/changeset.rb:27:in `update'
    from /builds/gitlab-org/gitlab/vendor/ruby/2.7.0/gems/undercover-0.4.4/lib/undercover.rb:34:in `initialize'
    from /builds/gitlab-org/gitlab/vendor/ruby/2.7.0/gems/undercover-0.4.4/lib/undercover/cli.rb:26:in `new'
    from /builds/gitlab-org/gitlab/vendor/ruby/2.7.0/gems/undercover-0.4.4/lib/undercover/cli.rb:26:in `run_report'
    from /builds/gitlab-org/gitlab/vendor/ruby/2.7.0/gems/undercover-0.4.4/lib/undercover/cli.rb:21:in `run'
    from /builds/gitlab-org/gitlab/vendor/ruby/2.7.0/gems/undercover-0.4.4/bin/undercover:12:in `block in <top (required)>'
    from /usr/local/lib/ruby/2.7.0/benchmark.rb:308:in `realtime'
    from /builds/gitlab-org/gitlab/vendor/ruby/2.7.0/gems/undercover-0.4.4/bin/undercover:11:in `<top (required)>'
    from /builds/gitlab-org/gitlab/vendor/ruby/2.7.0/bin/undercover:25:in `load'
    from /builds/gitlab-org/gitlab/vendor/ruby/2.7.0/bin/undercover:25:in `<top (required)>'
    from /usr/local/lib/ruby/gems/2.7.0/gems/bundler-2.3.15/lib/bundler/cli/exec.rb:58:in `load'
    from /usr/local/lib/ruby/gems/2.7.0/gems/bundler-2.3.15/lib/bundler/cli/exec.rb:58:in `kernel_load'
    from /usr/local/lib/ruby/gems/2.7.0/gems/bundler-2.3.15/lib/bundler/cli/exec.rb:23:in `run'
    from /usr/local/lib/ruby/gems/2.7.0/gems/bundler-2.3.15/lib/bundler/cli.rb:483:in `exec'
    from /usr/local/lib/ruby/gems/2.7.0/gems/bundler-2.3.15/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
    from /usr/local/lib/ruby/gems/2.7.0/gems/bundler-2.3.15/lib/bundler/vendor/thor/lib/thor/invocation.rb:127:in `invoke_command'
    from /usr/local/lib/ruby/gems/2.7.0/gems/bundler-2.3.15/lib/bundler/vendor/thor/lib/thor.rb:392:in `dispatch'
    from /usr/local/lib/ruby/gems/2.7.0/gems/bundler-2.3.15/lib/bundler/cli.rb:31:in `dispatch'
    from /usr/local/lib/ruby/gems/2.7.0/gems/bundler-2.3.15/lib/bundler/vendor/thor/lib/thor/base.rb:485:in `start'
    from /usr/local/lib/ruby/gems/2.7.0/gems/bundler-2.3.15/lib/bundler/cli.rb:25:in `start'
    from /usr/local/lib/ruby/gems/2.7.0/gems/bundler-2.3.15/exe/bundle:48:in `block in <top (required)>'
    from /usr/local/lib/ruby/gems/2.7.0/gems/bundler-2.3.15/lib/bundler/friendly_errors.rb:117:in `with_friendly_errors'
    from /usr/local/lib/ruby/gems/2.7.0/gems/bundler-2.3.15/exe/bundle:36:in `<top (required)>'
    from /usr/local/bin/bundle:23:in `load'
    from /usr/local/bin/bundle:23:in `<main>'

From https://gitlab.com/gitlab-org/gitlab/-/jobs/2627986054

Steps to reproduce

  1. Setup merged result pipelines on a GitLab project (https://docs.gitlab.com/ee/ci/pipelines/merged_results_pipelines.html)
  2. Create a merge request
  3. In the CI, run undercover -c ${CI_MERGE_REQUEST_TARGET_BRANCH_SHA}
  4. If the merge request's branch is old enough, it should fail with the above error.

https://github.com/libgit2/rugged/issues/846#issuecomment-983047575 seems related.

kuahyeow commented 2 years ago

I did manage to reproduce this locally as well - will post these steps soon.

We worked around this with the following monkey-patch

#!/usr/bin/env ruby

# frozen_string_literal: true

require 'undercover'

module Undercover
  class Changeset
    # Rugged merge_base complains when graft/shallow
    # (https://github.com/libgit2/rugged/issues/846)
    #
    # So we assume we provide the merge-base ourself. Modified from
    # https://github.com/grodowski/undercover/blob/32e62f66682ee566032b5970437ed2934ef29701/lib/undercover/changeset.rb#L74-L78
    def compare_base_obj
      return unless compare_base

      repo.lookup(compare_base.to_s)
    end
  end
end

compare_base = ARGV[0]
compare_base ||= IO.popen(%w(git merge-base origin/master HEAD)) { |p| p.read.chomp }

result = Undercover::CLI.run(%W(-c #{compare_base}))
exit result

I'm happy to draft up some PR but unsure if this should be another configuration option, or something else.

grodowski commented 2 years ago

Thanks for reporting this @kuahyeow, I remember getting compare_base_obj to work wasn't so obvious and I am aware that merge_base may stop working for shallow clones.

I'm happy to draft up some PR but unsure if this should be another configuration option, or something else.

I appreciate it and would be happy to help too. My initial thought is that ideally this wouldn't require any extra configuration, given that it's possible to infer what has been passed to --compare? If it's a ref, just look it up like in your workaround and only otherwise scan for the merge_base. I don't know it's doable with the current rugged API and will be looking more into it

kuahyeow commented 1 year ago

Thanks for reporting this @kuahyeow, I remember getting compare_base_obj to work wasn't so obvious and I am aware that merge_base may stop working for shallow clones.

I'm happy to draft up some PR but unsure if this should be another configuration option, or something else.

I appreciate it and would be happy to help too. My initial thought is that ideally this wouldn't require any extra configuration, given that it's possible to infer what has been passed to --compare? If it's a ref, just look it up like in your workaround and only otherwise scan for the merge_base. I don't know it's doable with the current rugged API and will be looking more into it

@grodowski In GitLab's case, we pass in a SHA to --compare and not a ref. So not sure that will work.

For example, GitLab CI provides a variable called CI_MERGE_REQUEST_TARGET_BRANCH_SHA which is already the merge-base of the feature branch, and the main branch. It's value looks like 91a9efed376560f1f4ce35f350bfc52520cbbc7b.

Possible ideas:

  1. Change --compare to never get merge-base itself, require user to do it themself (breaking change)
  2. Provide an option --[no-]compare-merge-base. If --no-compare-merge-base, undercover skips getting the merge base from --compare. If --compare-merge-base, undercover gets the merge base from --compare as per now.
grodowski commented 2 weeks ago

I made some recent changes to UndercoverCI to use shallow clones for a performance boost, which led me back here. I started a branch (https://github.com/grodowski/undercover/commit/31fbda6f5393d7f270bdbfdce17279cb027d2782) where compare_base_obj can fall back to the specific compare value if the merge base doesn't exist.

It looks like 1. proposed by @kuahyeow and so far I haven't considered adding new CLI options. Will probably ship this in a new major version after adding some test coverage.