diff --git a/Gemfile.lock b/Gemfile.lock index 1d85662..55e951d 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - code_ownership (1.29.2) + code_ownership (1.29.3) code_teams (~> 1.0) packs sorbet-runtime @@ -15,7 +15,7 @@ GEM coderay (1.1.3) diff-lcs (1.4.4) method_source (1.0.0) - packs (0.0.5) + packs (0.0.6) sorbet-runtime parser (3.1.2.0) ast (~> 2.4.1) diff --git a/code_ownership.gemspec b/code_ownership.gemspec index 5755328..322a0d1 100644 --- a/code_ownership.gemspec +++ b/code_ownership.gemspec @@ -1,6 +1,6 @@ Gem::Specification.new do |spec| spec.name = "code_ownership" - spec.version = '1.29.2' + spec.version = '1.29.3' spec.authors = ['Gusto Engineers'] spec.email = ['dev@gusto.com'] spec.summary = 'A gem to help engineering teams declare ownership of code' diff --git a/lib/code_ownership.rb b/lib/code_ownership.rb index 457c6db..24c0ab3 100644 --- a/lib/code_ownership.rb +++ b/lib/code_ownership.rb @@ -58,7 +58,6 @@ def for_team(team) ownership_information += ownership_for_mapper end - ownership_information << "" end @@ -93,7 +92,25 @@ def validate!( # first line that corresponds to a file with assigned ownership sig { params(backtrace: T.nilable(T::Array[String]), excluded_teams: T::Array[::CodeTeams::Team]).returns(T.nilable(::CodeTeams::Team)) } def for_backtrace(backtrace, excluded_teams: []) - return unless backtrace + first_owned_file_for_backtrace(backtrace, excluded_teams: excluded_teams)&.first + end + + # Given a backtrace from either `Exception#backtrace` or `caller`, find the + # first owned file in it, useful for figuring out which file is being blamed. + sig { params(backtrace: T.nilable(T::Array[String]), excluded_teams: T::Array[::CodeTeams::Team]).returns(T.nilable([::CodeTeams::Team, String])) } + def first_owned_file_for_backtrace(backtrace, excluded_teams: []) + backtrace_with_ownership(backtrace).each do |(team, file)| + if team && !excluded_teams.include?(team) + return [team, file] + end + end + + nil + end + + sig { params(backtrace: T.nilable(T::Array[String])).returns(T::Enumerable[[T.nilable(::CodeTeams::Team), String]]) } + def backtrace_with_ownership(backtrace) + return [] unless backtrace # The pattern for a backtrace hasn't changed in forever and is considered # stable: https://github.com/ruby/ruby/blob/trunk/vm_backtrace.c#L303-L317 @@ -110,18 +127,19 @@ def for_backtrace(backtrace, excluded_teams: []) `(?.*)' # Matches "`block (3 levels) in create'" \z}x - backtrace.each do |line| + backtrace.lazy.filter_map do |line| match = line.match(backtrace_line) + next unless match - if match - team = CodeOwnership.for_file(T.must(match[:file])) - if team && !excluded_teams.include?(team) - return team - end - end + file = T.must(match[:file]) + + [ + CodeOwnership.for_file(file), + file, + ] end - nil end + private_class_method(:backtrace_with_ownership) sig { params(klass: T.nilable(T.any(Class, Module))).returns(T.nilable(::CodeTeams::Team)) } def for_class(klass) diff --git a/spec/lib/code_ownership_spec.rb b/spec/lib/code_ownership_spec.rb index 79bf417..e97eb6b 100644 --- a/spec/lib/code_ownership_spec.rb +++ b/spec/lib/code_ownership_spec.rb @@ -705,22 +705,13 @@ end describe '.for_backtrace' do - def prevent_false_positive! - # The above code should raise, and we should never arrive at this next expectation. - # This is just to protect against a case where we have a false-postive test because the above does not raise. - expect(true).to eq false # rubocop:disable RSpec/ExpectActual - end - before do create_files_with_defined_classe end context 'excluded_teams is not passed in as an input parameter' do it 'finds the right team' do - begin # rubocop:disable Style/RedundantBegin - MyFile.raise_error - prevent_false_positive! - rescue StandardError => ex + expect { MyFile.raise_error }.to raise_error do |ex| expect(CodeOwnership.for_backtrace(ex.backtrace)).to eq CodeTeams.find('Bar') end end @@ -728,10 +719,7 @@ def prevent_false_positive! context 'excluded_teams is passed in as an input parameter' do it 'ignores the first part of the stack trace and finds the next viable owner' do - begin # rubocop:disable Style/RedundantBegin - MyFile.raise_error - prevent_false_positive! - rescue StandardError => ex + expect { MyFile.raise_error }.to raise_error do |ex| team_to_exclude = CodeTeams.find('Bar') expect(CodeOwnership.for_backtrace(ex.backtrace, excluded_teams: [team_to_exclude])).to eq CodeTeams.find('Foo') end @@ -739,6 +727,38 @@ def prevent_false_positive! end end + describe '.first_owned_file_for_backtrace' do + before do + create_files_with_defined_classe + end + + + context 'excluded_teams is not passed in as an input parameter' do + it 'finds the right team' do + expect { MyFile.raise_error }.to raise_error do |ex| + expect(CodeOwnership.first_owned_file_for_backtrace(ex.backtrace)).to eq [CodeTeams.find('Bar'), 'app/my_error.rb'] + end + end + end + + context 'excluded_teams is not passed in as an input parameter' do + it 'finds the right team' do + expect { MyFile.raise_error }.to raise_error do |ex| + team_to_exclude = CodeTeams.find('Bar') + expect(CodeOwnership.first_owned_file_for_backtrace(ex.backtrace, excluded_teams: [team_to_exclude])).to eq [CodeTeams.find('Foo'), 'app/my_file.rb'] + end + end + end + + context 'when nothing is owned' do + it 'returns nil' do + expect { raise 'opsy' }.to raise_error do |ex| + expect(CodeOwnership.first_owned_file_for_backtrace(ex.backtrace)).to be_nil + end + end + end + end + describe '.for_class' do before { create_files_with_defined_classe }