diff --git a/lib/danger-packwerk.rb b/lib/danger-packwerk.rb index 6bb1417..19837f0 100644 --- a/lib/danger-packwerk.rb +++ b/lib/danger-packwerk.rb @@ -15,4 +15,6 @@ module DangerPackwerk require 'danger-packwerk/check/default_formatter' require 'danger-packwerk/update/offenses_formatter' require 'danger-packwerk/update/default_formatter' + require 'danger-packwerk/pks_offense' + require 'danger-packwerk/pks_wrapper' end diff --git a/lib/danger-packwerk/danger_packwerk.rb b/lib/danger-packwerk/danger_packwerk.rb index af3a472..398e548 100644 --- a/lib/danger-packwerk/danger_packwerk.rb +++ b/lib/danger-packwerk/danger_packwerk.rb @@ -6,6 +6,8 @@ require 'parse_packwerk' require 'sorbet-runtime' require 'danger-packwerk/packwerk_wrapper' +require 'danger-packwerk/pks_wrapper' +require 'danger-packwerk/pks_offense' require 'danger-packwerk/private/git' module DangerPackwerk @@ -18,7 +20,8 @@ class DangerPackwerk < Danger::Plugin # especially given all violations should fail the build anyways. # We set a max (rather than unlimited) to avoid GitHub rate limiting and general spam if a PR does some sort of mass rename. DEFAULT_MAX_COMMENTS = 15 - OnFailure = T.type_alias { T.proc.params(offenses: T::Array[Packwerk::ReferenceOffense]).void } + # Support both legacy Packwerk::ReferenceOffense and new PksOffense types + OnFailure = T.type_alias { T.proc.params(offenses: T::Array[T.any(Packwerk::ReferenceOffense, PksOffense)]).void } DEFAULT_ON_FAILURE = T.let(->(offenses) {}, OnFailure) DEFAULT_FAIL = false DEFAULT_FAILURE_MESSAGE = 'Packwerk violations were detected! Please resolve them to unblock the build.' @@ -101,54 +104,55 @@ def check( current_comment_count = 0 - packwerk_reference_offenses = PackwerkWrapper.get_offenses_for_files(targeted_files.to_a).compact + pks_offenses = PksWrapper.get_offenses_for_files(targeted_files.to_a) renamed_files = git_filesystem.renamed_files.map { |before_after_file| before_after_file[:after] } - packwerk_reference_offenses_to_care_about = packwerk_reference_offenses.reject do |packwerk_reference_offense| - constant_name = packwerk_reference_offense.reference.constant.name + offenses_to_care_about = pks_offenses.reject do |offense| + constant_name = offense.reference.constant.name filepath_that_defines_this_constant = Private.constant_resolver.resolve(constant_name)&.location # Ignore references that have been renamed renamed_files.include?(filepath_that_defines_this_constant) || # Ignore violations that are not in the allow-list of violation types to leave comments for - !violation_types.include?(packwerk_reference_offense.violation_type) + !violation_types.include?(offense.violation_type) end # We group by the constant name, line number, and reference path. Any offenses with these same values should only differ on what type of violation # they are (privacy or dependency). We put privacy and dependency violation messages in the same comment since they would occur on the same line. - packwerk_reference_offenses_to_care_about.group_by do |packwerk_reference_offense| + offenses_to_care_about.group_by do |offense| case grouping_strategy when CommentGroupingStrategy::PerConstantPerLocation [ - packwerk_reference_offense.reference.constant.name, - packwerk_reference_offense.location&.line, - packwerk_reference_offense.reference.relative_path + offense.reference.constant.name, + offense.location.line, + offense.reference.relative_path ] when CommentGroupingStrategy::PerConstantPerPack [ - packwerk_reference_offense.reference.constant.name, - ParsePackwerk.package_from_path(packwerk_reference_offense.reference.relative_path) + offense.reference.constant.name, + ParsePackwerk.package_from_path(offense.reference.relative_path) ] else T.absurd(grouping_strategy) end - end.each_value do |unique_packwerk_reference_offenses| + end.each_value do |unique_offenses| break if current_comment_count >= max_comments current_comment_count += 1 - reference_offense = T.must(unique_packwerk_reference_offenses.first) - line_number = reference_offense.location&.line - referencing_file = reference_offense.reference.relative_path + offense = T.must(unique_offenses.first) + line_number = offense.location.line + referencing_file = offense.reference.relative_path - message = offenses_formatter.format_offenses(unique_packwerk_reference_offenses, repo_link, org_name, repo_url_builder: repo_url_builder) + # PksOffense adapters provide Packwerk::ReferenceOffense-compatible interface + message = offenses_formatter.format_offenses(T.unsafe(unique_offenses), repo_link, org_name, repo_url_builder: repo_url_builder) markdown(message, file: git_filesystem.convert_to_filesystem(referencing_file), line: line_number) end if current_comment_count > 0 fail(failure_message) if fail_build - on_failure.call(packwerk_reference_offenses) + on_failure.call(pks_offenses) end end end diff --git a/lib/danger-packwerk/pks_offense.rb b/lib/danger-packwerk/pks_offense.rb new file mode 100644 index 0000000..e884e28 --- /dev/null +++ b/lib/danger-packwerk/pks_offense.rb @@ -0,0 +1,146 @@ +# typed: strict + +require 'json' + +module DangerPackwerk + # + # PksOffense represents a violation from pks JSON output. + # It is designed to have a compatible interface with BasicReferenceOffense + # so it can be used with the Update::OffensesFormatter. + # + # It also provides adapter objects (reference, location) for compatibility + # with Packwerk::ReferenceOffense interface used by Check::OffensesFormatter. + # + class PksOffense < T::Struct + extend T::Sig + + const :violation_type, String + const :file, String + const :line, Integer + const :column, Integer + const :constant_name, String + const :referencing_pack_name, String + const :defining_pack_name, String + const :strict, T::Boolean + const :message, String + + # + # Adapter classes for Packwerk::ReferenceOffense compatibility + # These allow PksOffense to be used with Check::OffensesFormatter + # + class ConstantAdapter < T::Struct + const :name, String + const :location, String + const :package, T.untyped # Returns an object with .name method + end + + class PackageAdapter < T::Struct + const :name, String + end + + class ReferenceAdapter < T::Struct + const :relative_path, String + const :constant, ConstantAdapter + end + + class LocationAdapter < T::Struct + const :line, Integer + end + + # Adapter for Packwerk::ReferenceOffense.reference + sig { returns(ReferenceAdapter) } + def reference + package_adapter = PackageAdapter.new(name: defining_pack_name) + constant_adapter = ConstantAdapter.new( + name: constant_name, + location: file, # Best approximation - pks doesn't provide constant definition location + package: package_adapter + ) + ReferenceAdapter.new( + relative_path: file, + constant: constant_adapter + ) + end + + # Adapter for Packwerk::ReferenceOffense.location + sig { returns(LocationAdapter) } + def location + LocationAdapter.new(line: line) + end + + # Alias methods for compatibility with BasicReferenceOffense interface + sig { returns(String) } + def class_name + constant_name + end + + sig { returns(String) } + def type + violation_type + end + + sig { returns(String) } + def to_package_name + defining_pack_name + end + + sig { returns(String) } + def from_package_name + referencing_pack_name + end + + sig { returns(T::Boolean) } + def privacy? + violation_type == PRIVACY_VIOLATION_TYPE + end + + sig { returns(T::Boolean) } + def dependency? + violation_type == DEPENDENCY_VIOLATION_TYPE + end + + sig { params(other: PksOffense).returns(T::Boolean) } + def ==(other) + other.constant_name == constant_name && + other.file == file && + other.defining_pack_name == defining_pack_name && + other.violation_type == violation_type + end + + sig { params(other: PksOffense).returns(T::Boolean) } + def eql?(other) + self == other + end + + sig { returns(Integer) } + def hash + [constant_name, file, defining_pack_name, violation_type].hash + end + + class << self + extend T::Sig + + sig { params(json_string: String).returns(T::Array[PksOffense]) } + def from_json(json_string) + data = JSON.parse(json_string) + offenses = data['offenses'] || [] + offenses.map { |offense| from_hash(offense) } + end + + sig { params(hash: T::Hash[String, T.untyped]).returns(PksOffense) } + def from_hash(hash) + PksOffense.new( + violation_type: hash.fetch('violation_type'), + file: hash.fetch('file'), + line: hash.fetch('line'), + column: hash.fetch('column'), + constant_name: hash.fetch('constant_name'), + referencing_pack_name: hash.fetch('referencing_pack_name'), + defining_pack_name: hash.fetch('defining_pack_name'), + strict: hash.fetch('strict', false), + message: hash.fetch('message', '') + ) + end + end + end +end diff --git a/lib/danger-packwerk/pks_wrapper.rb b/lib/danger-packwerk/pks_wrapper.rb new file mode 100644 index 0000000..ebed153 --- /dev/null +++ b/lib/danger-packwerk/pks_wrapper.rb @@ -0,0 +1,39 @@ +# typed: strict + +require 'open3' +require 'json' + +module DangerPackwerk + class PksWrapper + extend T::Sig + + class PksBinaryNotFoundError < StandardError; end + + sig { params(files: T::Array[String]).returns(T::Array[PksOffense]) } + def self.get_offenses_for_files(files) + return [] if files.empty? + + stdout, stderr, _status = run_pks_check(files) + + if stderr.include?('command not found') || stderr.include?('No such file or directory') + raise PksBinaryNotFoundError, 'pks binary not found. Please install pks to use this feature.' + end + + PksOffense.from_json(stdout) + end + + sig { params(files: T::Array[String]).returns([String, String, Process::Status]) } + def self.run_pks_check(files) + require 'shellwords' + escaped_files = files.map { |f| Shellwords.escape(f) }.join(' ') + command = "pks check --output-format json #{escaped_files}" + Open3.capture3(command) + end + + sig { returns(T::Boolean) } + def self.pks_available? + _, _, status = Open3.capture3('which', 'pks') + !!status.success? + end + end +end diff --git a/spec/danger_packwerk/danger_packwerk_spec.rb b/spec/danger_packwerk/danger_packwerk_spec.rb index f239148..9d61e92 100644 --- a/spec/danger_packwerk/danger_packwerk_spec.rb +++ b/spec/danger_packwerk/danger_packwerk_spec.rb @@ -27,42 +27,28 @@ module DangerPackwerk context 'using inputted formatter' do let(:packwerk) { dangerfile.packwerk } - let(:depended_on_constant) do - sorbet_double(Packwerk::ConstantContext, location: 'packs/referencing_pack/some_file.rb', package: double(name: 'packs/referencing_pack'), name: '::SomeFile') - end - let(:constant) do - sorbet_double(Packwerk::ConstantContext, location: 'packs/some_pack/private_constant.rb', package: double(name: 'packs/some_pack'), name: '::PrivateConstant') - end let(:generic_dependency_violation) do - sorbet_double( - Packwerk::ReferenceOffense, - reference: depended_on_reference, + build_pks_offense( violation_type: ::DangerPackwerk::DEPENDENCY_VIOLATION_TYPE, - message: 'Vanilla message about dependency violations', - location: Packwerk::Node::Location.new(12, 5) + file: 'packs/some_pack/my_file.rb', + line: 12, + column: 5, + constant_name: '::SomeFile', + referencing_pack_name: 'packs/some_pack', + defining_pack_name: 'packs/referencing_pack', + message: 'Vanilla message about dependency violations' ) end let(:generic_privacy_violation) do - sorbet_double( - Packwerk::ReferenceOffense, - reference: reference, + build_pks_offense( violation_type: ::DangerPackwerk::PRIVACY_VIOLATION_TYPE, - message: 'Vanilla message about privacy violations', - location: Packwerk::Node::Location.new(12, 5) - ) - end - let(:depended_on_reference) do - sorbet_double( - Packwerk::Reference, - relative_path: 'packs/some_pack/my_file.rb', - constant: depended_on_constant - ) - end - let(:reference) do - sorbet_double( - Packwerk::Reference, - relative_path: 'packs/referencing_pack/some_file.rb', - constant: constant + file: 'packs/referencing_pack/some_file.rb', + line: 12, + column: 5, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_pack', + defining_pack_name: 'packs/some_pack', + message: 'Vanilla message about privacy violations' ) end let(:plugin) { packwerk } @@ -93,12 +79,11 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) enforce_dependencies: true enforce_privacy: true YML - allow_any_instance_of(Packwerk::Cli).to receive(:execute_command).with(['check', *files_for_packwerk]) - allow_any_instance_of(PackwerkWrapper::OffensesAggregatorFormatter).to receive(:aggregated_offenses).and_return(offenses) + allow(PksWrapper).to receive(:get_offenses_for_files).and_return(offenses) end - context 'when there are syntax errors in analyzed files' do - let(:offenses) { [sorbet_double(Packwerk::Parsers::ParseResult)] } + context 'when pks returns no violations (e.g., syntax errors in files)' do + let(:offenses) { [] } it 'exits gracefully' do packwerk_check @@ -113,7 +98,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) let(:modified_files) { [write_file('frontend/javascript/some_file.js').to_s] } it 'leaves an inline comment helping the user figure out what to do next' do - expect_any_instance_of(Packwerk::Cli).not_to receive(:execute_command) + expect(PksWrapper).not_to receive(:get_offenses_for_files) packwerk_check expect(dangerfile.status_report[:warnings]).to be_empty expect(dangerfile.status_report[:errors]).to be_empty @@ -133,7 +118,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) expect(actual_markdowns.count).to eq 1 actual_markdown = actual_markdowns.first expect(actual_markdown.message).to eq(<<~MSG.chomp) - Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) MSG expect(actual_markdown.line).to eq 12 expect(actual_markdown.file).to eq 'packs/referencing_pack/some_file.rb' @@ -154,7 +139,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) expect(actual_markdowns.count).to eq 1 actual_markdown = actual_markdowns.first expect(actual_markdown.message).to eq(<<~MSG.chomp) - Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) MSG expect(actual_markdown.line).to eq 12 expect(actual_markdown.file).to eq 'packs/referencing_pack/some_file.rb' @@ -174,7 +159,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) expect(actual_markdowns.count).to eq 1 actual_markdown = actual_markdowns.first expect(actual_markdown.message).to eq(<<~MSG.chomp) - Vanilla message about dependency violations [::SomeFile](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) + Vanilla message about dependency violations [::SomeFile](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/my_file.rb) MSG expect(actual_markdown.line).to eq 12 expect(actual_markdown.file).to eq 'packs/some_pack/my_file.rb' @@ -194,7 +179,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) actual_markdown = actual_markdowns[0] expect(actual_markdown.message).to eq(<<~MSG.chomp) - Vanilla message about dependency violations [::SomeFile](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) + Vanilla message about dependency violations [::SomeFile](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/my_file.rb) MSG expect(actual_markdown.line).to eq 12 expect(actual_markdown.file).to eq 'packs/some_pack/my_file.rb' @@ -202,7 +187,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) actual_markdown = actual_markdowns[1] expect(actual_markdown.message).to eq(<<~MSG.chomp) - Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) MSG expect(actual_markdown.line).to eq 12 expect(actual_markdown.file).to eq 'packs/referencing_pack/some_file.rb' @@ -227,7 +212,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) actual_markdown = actual_markdowns[0] expect(actual_markdown.message).to eq(<<~MSG.chomp) - Vanilla message about dependency violations [::SomeFile](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) + Vanilla message about dependency violations [::SomeFile](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/my_file.rb) MSG expect(actual_markdown.line).to eq 12 expect(actual_markdown.file).to eq "#{root_path}packs/some_pack/my_file.rb" @@ -235,7 +220,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) actual_markdown = actual_markdowns[1] expect(actual_markdown.message).to eq(<<~MSG.chomp) - Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) MSG expect(actual_markdown.line).to eq 12 expect(actual_markdown.file).to eq "#{root_path}packs/referencing_pack/some_file.rb" @@ -261,7 +246,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) expect(actual_markdowns.count).to eq 1 actual_markdown = actual_markdowns.first expect(actual_markdown.message).to eq(<<~MSG.chomp) - Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) MSG expect(actual_markdown.line).to eq 12 expect(actual_markdown.file).to eq "#{root_path}packs/referencing_pack/some_file.rb" @@ -273,29 +258,27 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) context 'when there are violations on the same constant' do context 'with default (per constant per line)' do context 'on the same line' do - let(:reference) do - sorbet_double( - Packwerk::Reference, - relative_path: 'packs/referencing_pack/some_file.rb', - constant: constant - ) - end - let(:offenses) do [ - sorbet_double( - Packwerk::ReferenceOffense, - reference: reference, + build_pks_offense( violation_type: ::DangerPackwerk::PRIVACY_VIOLATION_TYPE, - message: 'Vanilla message about privacy violations', - location: Packwerk::Node::Location.new(12, 15) + file: 'packs/referencing_pack/some_file.rb', + line: 12, + column: 15, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_pack', + defining_pack_name: 'packs/some_pack', + message: 'Vanilla message about privacy violations' ), - sorbet_double( - Packwerk::ReferenceOffense, - reference: reference, + build_pks_offense( violation_type: ::DangerPackwerk::DEPENDENCY_VIOLATION_TYPE, - message: 'Vanilla message about dependency violations', - location: Packwerk::Node::Location.new(12, 15) + file: 'packs/referencing_pack/some_file.rb', + line: 12, + column: 15, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_pack', + defining_pack_name: 'packs/some_pack', + message: 'Vanilla message about dependency violations' ) ] end @@ -308,9 +291,9 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) expect(actual_markdowns.count).to eq 1 actual_markdown = actual_markdowns.first expect(actual_markdown.message).to eq(<<~MSG.chomp) - Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) - Vanilla message about dependency violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about dependency violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) MSG expect(actual_markdown.line).to eq 12 expect(actual_markdown.file).to eq 'packs/referencing_pack/some_file.rb' @@ -319,29 +302,27 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) end context 'within the same file' do - let(:reference) do - sorbet_double( - Packwerk::Reference, - relative_path: 'packs/referencing_pack/some_file.rb', - constant: constant - ) - end - let(:offenses) do [ - sorbet_double( - Packwerk::ReferenceOffense, - reference: reference, + build_pks_offense( violation_type: ::DangerPackwerk::PRIVACY_VIOLATION_TYPE, - message: 'Vanilla message about privacy violations', - location: Packwerk::Node::Location.new(12, 5) + file: 'packs/referencing_pack/some_file.rb', + line: 12, + column: 5, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_pack', + defining_pack_name: 'packs/some_pack', + message: 'Vanilla message about privacy violations' ), - sorbet_double( - Packwerk::ReferenceOffense, - reference: reference, + build_pks_offense( violation_type: ::DangerPackwerk::PRIVACY_VIOLATION_TYPE, - message: 'Vanilla message about privacy violations', - location: Packwerk::Node::Location.new(22, 5) + file: 'packs/referencing_pack/some_file.rb', + line: 22, + column: 5, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_pack', + defining_pack_name: 'packs/some_pack', + message: 'Vanilla message about privacy violations' ) ] end @@ -356,7 +337,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) first_actual_markdown = actual_markdowns.first expect(first_actual_markdown.message).to eq(<<~MSG.chomp) - Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) MSG expect(first_actual_markdown.line).to eq 12 expect(first_actual_markdown.file).to eq 'packs/referencing_pack/some_file.rb' @@ -364,7 +345,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) second_actual_markdown = actual_markdowns.last expect(second_actual_markdown.message).to eq(<<~MSG.chomp) - Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) MSG expect(second_actual_markdown.line).to eq 22 expect(second_actual_markdown.file).to eq 'packs/referencing_pack/some_file.rb' @@ -375,27 +356,25 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) context 'within the same pack' do let(:offenses) do [ - sorbet_double( - Packwerk::ReferenceOffense, - reference: sorbet_double( - Packwerk::Reference, - relative_path: 'packs/referencing_pack/some_file.rb', - constant: constant - ), + build_pks_offense( violation_type: ::DangerPackwerk::PRIVACY_VIOLATION_TYPE, - message: 'Vanilla message about privacy violations', - location: Packwerk::Node::Location.new(12, 5) + file: 'packs/referencing_pack/some_file.rb', + line: 12, + column: 5, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_pack', + defining_pack_name: 'packs/some_pack', + message: 'Vanilla message about privacy violations' ), - sorbet_double( - Packwerk::ReferenceOffense, - reference: sorbet_double( - Packwerk::Reference, - relative_path: 'packs/referencing_pack/some_other_file.rb', - constant: constant - ), + build_pks_offense( violation_type: ::DangerPackwerk::PRIVACY_VIOLATION_TYPE, - message: 'Vanilla message about privacy violations', - location: Packwerk::Node::Location.new(12, 5) + file: 'packs/referencing_pack/some_other_file.rb', + line: 12, + column: 5, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_pack', + defining_pack_name: 'packs/some_pack', + message: 'Vanilla message about privacy violations' ) ] end @@ -410,7 +389,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) first_actual_markdown = actual_markdowns.first expect(first_actual_markdown.message).to eq(<<~MSG.chomp) - Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) MSG expect(first_actual_markdown.line).to eq 12 expect(first_actual_markdown.file).to eq 'packs/referencing_pack/some_file.rb' @@ -418,7 +397,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) second_actual_markdown = actual_markdowns.last expect(second_actual_markdown.message).to eq(<<~MSG.chomp) - Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_other_file.rb) MSG expect(second_actual_markdown.line).to eq 12 expect(second_actual_markdown.file).to eq 'packs/referencing_pack/some_other_file.rb' @@ -429,27 +408,25 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) context 'across different packs' do let(:offenses) do [ - sorbet_double( - Packwerk::ReferenceOffense, - reference: sorbet_double( - Packwerk::Reference, - relative_path: 'packs/referencing_pack/some_file.rb', - constant: constant - ), + build_pks_offense( violation_type: ::DangerPackwerk::PRIVACY_VIOLATION_TYPE, - message: 'Vanilla message about privacy violations', - location: Packwerk::Node::Location.new(12, 5) + file: 'packs/referencing_pack/some_file.rb', + line: 12, + column: 5, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_pack', + defining_pack_name: 'packs/some_pack', + message: 'Vanilla message about privacy violations' ), - sorbet_double( - Packwerk::ReferenceOffense, - reference: sorbet_double( - Packwerk::Reference, - relative_path: 'packs/another_referencing_pack/some_file.rb', - constant: constant - ), + build_pks_offense( violation_type: ::DangerPackwerk::PRIVACY_VIOLATION_TYPE, - message: 'Vanilla message about privacy violations', - location: Packwerk::Node::Location.new(12, 5) + file: 'packs/another_referencing_pack/some_file.rb', + line: 12, + column: 5, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/another_referencing_pack', + defining_pack_name: 'packs/some_pack', + message: 'Vanilla message about privacy violations' ) ] end @@ -464,7 +441,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) first_actual_markdown = actual_markdowns.first expect(first_actual_markdown.message).to eq(<<~MSG.chomp) - Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) MSG expect(first_actual_markdown.line).to eq 12 expect(first_actual_markdown.file).to eq 'packs/referencing_pack/some_file.rb' @@ -472,7 +449,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) second_actual_markdown = actual_markdowns.last expect(second_actual_markdown.message).to eq(<<~MSG.chomp) - Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/another_referencing_pack/some_file.rb) MSG expect(second_actual_markdown.line).to eq 12 expect(second_actual_markdown.file).to eq 'packs/another_referencing_pack/some_file.rb' @@ -490,29 +467,27 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) end context 'on the same line' do - let(:reference) do - sorbet_double( - Packwerk::Reference, - relative_path: 'packs/referencing_pack/some_file.rb', - constant: constant - ) - end - let(:offenses) do [ - sorbet_double( - Packwerk::ReferenceOffense, - reference: reference, + build_pks_offense( violation_type: ::DangerPackwerk::PRIVACY_VIOLATION_TYPE, - message: 'Vanilla message about privacy violations', - location: Packwerk::Node::Location.new(12, 15) + file: 'packs/referencing_pack/some_file.rb', + line: 12, + column: 15, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_pack', + defining_pack_name: 'packs/some_pack', + message: 'Vanilla message about privacy violations' ), - sorbet_double( - Packwerk::ReferenceOffense, - reference: reference, + build_pks_offense( violation_type: ::DangerPackwerk::DEPENDENCY_VIOLATION_TYPE, - message: 'Vanilla message about dependency violations', - location: Packwerk::Node::Location.new(12, 15) + file: 'packs/referencing_pack/some_file.rb', + line: 12, + column: 15, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_pack', + defining_pack_name: 'packs/some_pack', + message: 'Vanilla message about dependency violations' ) ] end @@ -525,9 +500,9 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) expect(actual_markdowns.count).to eq 1 actual_markdown = actual_markdowns.first expect(actual_markdown.message).to eq(<<~MSG.chomp) - Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) - Vanilla message about dependency violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about dependency violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) MSG expect(actual_markdown.line).to eq 12 expect(actual_markdown.file).to eq 'packs/referencing_pack/some_file.rb' @@ -536,29 +511,27 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) end context 'within the same file' do - let(:reference) do - sorbet_double( - Packwerk::Reference, - relative_path: 'packs/referencing_pack/some_file.rb', - constant: constant - ) - end - let(:offenses) do [ - sorbet_double( - Packwerk::ReferenceOffense, - reference: reference, + build_pks_offense( violation_type: ::DangerPackwerk::PRIVACY_VIOLATION_TYPE, - message: 'Vanilla message about privacy violations', - location: Packwerk::Node::Location.new(12, 5) + file: 'packs/referencing_pack/some_file.rb', + line: 12, + column: 5, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_pack', + defining_pack_name: 'packs/some_pack', + message: 'Vanilla message about privacy violations' ), - sorbet_double( - Packwerk::ReferenceOffense, - reference: reference, + build_pks_offense( violation_type: ::DangerPackwerk::PRIVACY_VIOLATION_TYPE, - message: 'Vanilla message about privacy violations', - location: Packwerk::Node::Location.new(22, 5) + file: 'packs/referencing_pack/some_file.rb', + line: 22, + column: 5, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_pack', + defining_pack_name: 'packs/some_pack', + message: 'Vanilla message about privacy violations' ) ] end @@ -571,9 +544,9 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) expect(actual_markdowns.count).to eq 1 actual_markdown = actual_markdowns.first expect(actual_markdown.message).to eq(<<~MSG.chomp) - Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) - Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) MSG expect(actual_markdown.line).to eq 12 expect(actual_markdown.file).to eq 'packs/referencing_pack/some_file.rb' @@ -584,27 +557,25 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) context 'within the same pack' do let(:offenses) do [ - sorbet_double( - Packwerk::ReferenceOffense, - reference: sorbet_double( - Packwerk::Reference, - relative_path: 'packs/referencing_pack/some_file.rb', - constant: constant - ), + build_pks_offense( violation_type: ::DangerPackwerk::PRIVACY_VIOLATION_TYPE, - message: 'Vanilla message about privacy violations', - location: Packwerk::Node::Location.new(12, 5) + file: 'packs/referencing_pack/some_file.rb', + line: 12, + column: 5, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_pack', + defining_pack_name: 'packs/some_pack', + message: 'Vanilla message about privacy violations' ), - sorbet_double( - Packwerk::ReferenceOffense, - reference: sorbet_double( - Packwerk::Reference, - relative_path: 'packs/referencing_pack/some_other_file.rb', - constant: constant - ), + build_pks_offense( violation_type: ::DangerPackwerk::PRIVACY_VIOLATION_TYPE, - message: 'Vanilla message about privacy violations', - location: Packwerk::Node::Location.new(12, 5) + file: 'packs/referencing_pack/some_other_file.rb', + line: 12, + column: 5, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_pack', + defining_pack_name: 'packs/some_pack', + message: 'Vanilla message about privacy violations' ) ] end @@ -616,10 +587,11 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) actual_markdowns = dangerfile.status_report[:markdowns] expect(actual_markdowns.count).to eq 1 actual_markdown = actual_markdowns.first + # The formatter uses offenses.first.reference.constant.location for all URLs expect(actual_markdown.message).to eq(<<~MSG.chomp) - Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) - Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) MSG expect(actual_markdown.line).to eq 12 expect(actual_markdown.file).to eq 'packs/referencing_pack/some_file.rb' @@ -641,27 +613,25 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) let(:offenses) do [ - sorbet_double( - Packwerk::ReferenceOffense, - reference: sorbet_double( - Packwerk::Reference, - relative_path: 'packs/referencing_pack/some_file.rb', - constant: constant - ), + build_pks_offense( violation_type: ::DangerPackwerk::PRIVACY_VIOLATION_TYPE, - message: 'Vanilla message about privacy violations', - location: Packwerk::Node::Location.new(12, 5) + file: 'packs/referencing_pack/some_file.rb', + line: 12, + column: 5, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_pack', + defining_pack_name: 'packs/some_pack', + message: 'Vanilla message about privacy violations' ), - sorbet_double( - Packwerk::ReferenceOffense, - reference: sorbet_double( - Packwerk::Reference, - relative_path: 'packs/another_referencing_pack/some_file.rb', - constant: constant - ), + build_pks_offense( violation_type: ::DangerPackwerk::PRIVACY_VIOLATION_TYPE, - message: 'Vanilla message about privacy violations', - location: Packwerk::Node::Location.new(12, 5) + file: 'packs/another_referencing_pack/some_file.rb', + line: 12, + column: 5, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/another_referencing_pack', + defining_pack_name: 'packs/some_pack', + message: 'Vanilla message about privacy violations' ) ] end @@ -676,7 +646,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) first_actual_markdown = actual_markdowns.first expect(first_actual_markdown.message).to eq(<<~MSG.chomp) - Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb) MSG expect(first_actual_markdown.line).to eq 12 expect(first_actual_markdown.file).to eq 'packs/referencing_pack/some_file.rb' @@ -684,7 +654,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) second_actual_markdown = actual_markdowns.last expect(second_actual_markdown.message).to eq(<<~MSG.chomp) - Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb) + Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/another_referencing_pack/some_file.rb) MSG expect(second_actual_markdown.line).to eq 12 expect(second_actual_markdown.file).to eq 'packs/another_referencing_pack/some_file.rb' @@ -697,12 +667,15 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) context 'when there are 100 new violations when running packwerk check' do let(:offenses) do 100.times.to_a.map do |i| - sorbet_double( - Packwerk::ReferenceOffense, + build_pks_offense( violation_type: ::DangerPackwerk::DEPENDENCY_VIOLATION_TYPE, - reference: reference, - message: 'blah', - location: Packwerk::Node::Location.new(i, 5) + file: 'packs/referencing_pack/some_file.rb', + line: i, + column: 5, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_pack', + defining_pack_name: 'packs/some_pack', + message: 'blah' ) end end @@ -792,12 +765,21 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) ] end - let(:depended_on_constant) do - sorbet_double(Packwerk::ConstantContext, name: 'SomeClassWithNewName') + let(:offenses) do + [ + build_pks_offense( + violation_type: ::DangerPackwerk::DEPENDENCY_VIOLATION_TYPE, + file: 'packs/some_pack/my_file.rb', + line: 12, + column: 5, + constant_name: 'SomeClassWithNewName', + referencing_pack_name: 'packs/some_pack', + defining_pack_name: 'packs/referencing_pack', + message: 'Vanilla message about dependency violations' + ) + ] end - let(:offenses) { [generic_dependency_violation] } - it 'does not leave an inline comment' do packwerk_check expect(dangerfile.status_report[:warnings]).to be_empty @@ -808,12 +790,15 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) context 'when there is a new unknown violation when running packwerk check' do let(:unknown_violation) do - sorbet_double( - Packwerk::ReferenceOffense, - reference: reference, + build_pks_offense( violation_type: 'unknown', - message: 'Some unknown message', - location: Packwerk::Node::Location.new(12, 5) + file: 'packs/referencing_pack/some_file.rb', + line: 12, + column: 5, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_pack', + defining_pack_name: 'packs/some_pack', + message: 'Some unknown message' ) end let(:offenses) { [unknown_violation] } @@ -833,7 +818,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) actual_markdowns = dangerfile.status_report[:markdowns] expect(actual_markdowns.count).to eq 1 actual_markdown = actual_markdowns.first - expect(actual_markdown.message).to eq 'Some unknown message [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/some_pack/private_constant.rb)' + expect(actual_markdown.message).to eq 'Some unknown message [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb)' expect(actual_markdown.line).to eq 12 expect(actual_markdown.file).to eq 'packs/referencing_pack/some_file.rb' expect(actual_markdown.type).to eq :markdown @@ -865,8 +850,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) end end - allow_any_instance_of(Packwerk::Cli).to receive(:execute_command) - allow_any_instance_of(::DangerPackwerk::PackwerkWrapper::OffensesAggregatorFormatter).to receive(:aggregated_offenses).and_return(offenses) + allow(PksWrapper).to receive(:get_offenses_for_files).and_return(offenses) write_file('config/teams/product_infrastructure.yml', <<~YML) name: Product Infrastructure Backend @@ -891,30 +875,27 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) let(:danger_packwerk) { dangerfile.packwerk } let(:generic_privacy_violation) do - sorbet_double( - Packwerk::ReferenceOffense, + build_pks_offense( violation_type: ::DangerPackwerk::PRIVACY_VIOLATION_TYPE, - reference: reference, - location: Packwerk::Node::Location.new(12, 5) + file: 'packs/referencing_package/some_file.rb', + line: 12, + column: 5, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_package', + defining_pack_name: 'packs/gusto_slack', + message: '' ) end let(:generic_dependency_violation) do - sorbet_double( - Packwerk::ReferenceOffense, + build_pks_offense( violation_type: ::DangerPackwerk::DEPENDENCY_VIOLATION_TYPE, - reference: reference, - location: Packwerk::Node::Location.new(12, 5) - ) - end - let(:constant) do - sorbet_double(Packwerk::ConstantContext, package: slack_package, name: '::PrivateConstant', location: 'packs/gusto_slack/app/services/private_constant.rb') - end - let(:reference) do - sorbet_double( - Packwerk::Reference, - package: referencing_package, - relative_path: 'packs/referencing_package/some_file.rb', - constant: constant + file: 'packs/referencing_package/some_file.rb', + line: 12, + column: 5, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_package', + defining_pack_name: 'packs/gusto_slack', + message: '' ) end let(:referencing_package) { ParsePackwerk.find('packs/referencing_package') } @@ -936,7 +917,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) expected = <<~EXPECTED **Packwerk Violation** - Type: Privacy :lock: - - Constant: [`PrivateConstant`](https://github.com/MyOrg/my_repo/blob/my_branch/packs/gusto_slack/app/services/private_constant.rb) + - Constant: [`PrivateConstant`](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_package/some_file.rb) - Owning pack: packs/gusto_slack - Owned by [@MyOrg/product-infrastructure](https://github.com/orgs/MyOrg/teams/product-infrastructure/members) (Slack: [#prod-infra](https://slack.com/app_redirect?channel=prod-infra)) @@ -946,10 +927,10 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) - Does the code you are writing live in the right pack? - If not, try `bin/packs move packs/destination_pack packs/referencing_package/some_file.rb` - Does PrivateConstant live in the right pack? - - If not, try `bin/packs move packs/destination_pack packs/gusto_slack/app/services/private_constant.rb` + - If not, try `bin/packs move packs/destination_pack packs/referencing_package/some_file.rb` - Does API in packs/gusto_slack/public support this use case? - If not, can we work with [@MyOrg/product-infrastructure](https://github.com/orgs/MyOrg/teams/product-infrastructure/members) to create and use a public API? - - If `PrivateConstant` should already be public, try `bin/packs make_public packs/gusto_slack/app/services/private_constant.rb`. + - If `PrivateConstant` should already be public, try `bin/packs make_public packs/referencing_package/some_file.rb`. @@ -977,7 +958,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) expected = <<~EXPECTED **Packwerk Violation** - Type: Privacy :lock: - - Constant: [`PrivateConstant`](https://github.com/MyOrg/my_repo/blob/my_branch/packs/gusto_slack/app/services/private_constant.rb) + - Constant: [`PrivateConstant`](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_package/some_file.rb) - Owning pack: packs/gusto_slack - This pack is unowned. @@ -987,10 +968,10 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) - Does the code you are writing live in the right pack? - If not, try `bin/packs move packs/destination_pack packs/referencing_package/some_file.rb` - Does PrivateConstant live in the right pack? - - If not, try `bin/packs move packs/destination_pack packs/gusto_slack/app/services/private_constant.rb` + - If not, try `bin/packs move packs/destination_pack packs/referencing_package/some_file.rb` - Does API in packs/gusto_slack/public support this use case? - If not, can we work with the pack owner to create and use a public API? - - If `PrivateConstant` should already be public, try `bin/packs make_public packs/gusto_slack/app/services/private_constant.rb`. + - If `PrivateConstant` should already be public, try `bin/packs make_public packs/referencing_package/some_file.rb`. @@ -1020,7 +1001,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) expected = <<~EXPECTED **Packwerk Violation** - Type: Dependency :knot: - - Constant: [`PrivateConstant`](https://github.com/MyOrg/my_repo/blob/my_branch/packs/gusto_slack/app/services/private_constant.rb) + - Constant: [`PrivateConstant`](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_package/some_file.rb) - Owning pack: packs/gusto_slack - Owned by [@MyOrg/product-infrastructure](https://github.com/orgs/MyOrg/teams/product-infrastructure/members) (Slack: [#prod-infra](https://slack.com/app_redirect?channel=prod-infra)) @@ -1030,7 +1011,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) - Does the code you are writing live in the right pack? - If not, try `bin/packs move packs/destination_pack packs/referencing_package/some_file.rb` - Does PrivateConstant live in the right pack? - - If not, try `bin/packs move packs/destination_pack packs/gusto_slack/app/services/private_constant.rb` + - If not, try `bin/packs move packs/destination_pack packs/referencing_package/some_file.rb` - Do we actually want to depend on packs/gusto_slack? - If so, try `bin/packs add_dependency packs/referencing_package packs/gusto_slack` - If not, what can we change about the design so we do not have to depend on packs/gusto_slack? @@ -1061,7 +1042,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) expected = <<~EXPECTED **Packwerk Violation** - Type: Privacy :lock: + Dependency :knot: - - Constant: [`PrivateConstant`](https://github.com/MyOrg/my_repo/blob/my_branch/packs/gusto_slack/app/services/private_constant.rb) + - Constant: [`PrivateConstant`](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_package/some_file.rb) - Owning pack: packs/gusto_slack - Owned by [@MyOrg/product-infrastructure](https://github.com/orgs/MyOrg/teams/product-infrastructure/members) (Slack: [#prod-infra](https://slack.com/app_redirect?channel=prod-infra)) @@ -1071,13 +1052,13 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) - Does the code you are writing live in the right pack? - If not, try `bin/packs move packs/destination_pack packs/referencing_package/some_file.rb` - Does PrivateConstant live in the right pack? - - If not, try `bin/packs move packs/destination_pack packs/gusto_slack/app/services/private_constant.rb` + - If not, try `bin/packs move packs/destination_pack packs/referencing_package/some_file.rb` - Do we actually want to depend on packs/gusto_slack? - If so, try `bin/packs add_dependency packs/referencing_package packs/gusto_slack` - If not, what can we change about the design so we do not have to depend on packs/gusto_slack? - Does API in packs/gusto_slack/public support this use case? - If not, can we work with [@MyOrg/product-infrastructure](https://github.com/orgs/MyOrg/teams/product-infrastructure/members) to create and use a public API? - - If `PrivateConstant` should already be public, try `bin/packs make_public packs/gusto_slack/app/services/private_constant.rb`. + - If `PrivateConstant` should already be public, try `bin/packs make_public packs/referencing_package/some_file.rb`. @@ -1095,29 +1076,25 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) context 'within the same pack' do let(:offenses) do [ - sorbet_double( - Packwerk::ReferenceOffense, - reference: sorbet_double( - Packwerk::Reference, - package: referencing_package, - relative_path: 'packs/referencing_package/some_file.rb', - constant: constant - ), + build_pks_offense( violation_type: ::DangerPackwerk::PRIVACY_VIOLATION_TYPE, - message: 'Vanilla message about privacy violations', - location: Packwerk::Node::Location.new(12, 5) + file: 'packs/referencing_package/some_file.rb', + line: 12, + column: 5, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_package', + defining_pack_name: 'packs/gusto_slack', + message: 'Vanilla message about privacy violations' ), - sorbet_double( - Packwerk::ReferenceOffense, - reference: sorbet_double( - Packwerk::Reference, - package: referencing_package, - relative_path: 'packs/referencing_package/some_other_file.rb', - constant: constant - ), + build_pks_offense( violation_type: ::DangerPackwerk::PRIVACY_VIOLATION_TYPE, - message: 'Vanilla message about privacy violations', - location: Packwerk::Node::Location.new(12, 5) + file: 'packs/referencing_package/some_other_file.rb', + line: 12, + column: 5, + constant_name: '::PrivateConstant', + referencing_pack_name: 'packs/referencing_package', + defining_pack_name: 'packs/gusto_slack', + message: 'Vanilla message about privacy violations' ) ] end @@ -1134,7 +1111,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) expected = <<~EXPECTED **Packwerk Violation** - Type: Privacy :lock: - - Constant: [`PrivateConstant`](https://github.com/MyOrg/my_repo/blob/my_branch/packs/gusto_slack/app/services/private_constant.rb) + - Constant: [`PrivateConstant`](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_package/some_file.rb) - Owning pack: packs/gusto_slack - Owned by [@MyOrg/product-infrastructure](https://github.com/orgs/MyOrg/teams/product-infrastructure/members) (Slack: [#prod-infra](https://slack.com/app_redirect?channel=prod-infra)) @@ -1144,10 +1121,10 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) - Does the code you are writing live in the right pack? - If not, try `bin/packs move packs/destination_pack packs/referencing_package/some_file.rb` - Does PrivateConstant live in the right pack? - - If not, try `bin/packs move packs/destination_pack packs/gusto_slack/app/services/private_constant.rb` + - If not, try `bin/packs move packs/destination_pack packs/referencing_package/some_file.rb` - Does API in packs/gusto_slack/public support this use case? - If not, can we work with [@MyOrg/product-infrastructure](https://github.com/orgs/MyOrg/teams/product-infrastructure/members) to create and use a public API? - - If `PrivateConstant` should already be public, try `bin/packs make_public packs/gusto_slack/app/services/private_constant.rb`. + - If `PrivateConstant` should already be public, try `bin/packs make_public packs/referencing_package/some_file.rb`. @@ -1165,21 +1142,15 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) context 'when there are 100 new violations when running packwerk check' do let(:offenses) do 100.times.to_a.map do |i| - sorbet_double( - Packwerk::ReferenceOffense, + build_pks_offense( violation_type: ::DangerPackwerk::DEPENDENCY_VIOLATION_TYPE, - reference: sorbet_double( - Packwerk::Reference, - package: referencing_package, - relative_path: 'packs/referencing_package/some_file.rb', - constant: sorbet_double( - Packwerk::ConstantContext, - package: slack_package, - name: "::PrivateConstant#{i}", - location: 'packs/gusto_slack/app/services/private_constant.rb' - ) - ), - location: Packwerk::Node::Location.new(i, 5) + file: 'packs/referencing_package/some_file.rb', + line: i, + column: 5, + constant_name: "::PrivateConstant#{i}", + referencing_pack_name: 'packs/referencing_package', + defining_pack_name: 'packs/gusto_slack', + message: '' ) end end diff --git a/spec/danger_packwerk/pks_offense_spec.rb b/spec/danger_packwerk/pks_offense_spec.rb new file mode 100644 index 0000000..1786193 --- /dev/null +++ b/spec/danger_packwerk/pks_offense_spec.rb @@ -0,0 +1,250 @@ +require 'spec_helper' + +module DangerPackwerk + RSpec.describe PksOffense do + # PksOffense doesn't need Danger plugin context, but spec_helper includes it + let(:plugin) { dangerfile.packwerk } + let(:fixtures_path) { File.join(__dir__, '..', 'fixtures', 'pks_output') } + let(:privacy_offense_hash) do + { + 'violation_type' => 'privacy', + 'file' => 'packs/my_pack/app/models/user.rb', + 'line' => 42, + 'column' => 10, + 'constant_name' => '::OtherPack::PrivateClass', + 'referencing_pack_name' => 'packs/my_pack', + 'defining_pack_name' => 'packs/other_pack', + 'strict' => false, + 'message' => 'Privacy violation: ::OtherPack::PrivateClass is private' + } + end + + let(:dependency_offense_hash) do + { + 'violation_type' => 'dependency', + 'file' => 'packs/my_pack/app/services/user_service.rb', + 'line' => 15, + 'column' => 5, + 'constant_name' => '::ThirdPack::SomeConstant', + 'referencing_pack_name' => 'packs/my_pack', + 'defining_pack_name' => 'packs/third_pack', + 'strict' => true, + 'message' => 'Dependency violation: packs/my_pack does not declare packs/third_pack as a dependency' + } + end + + describe '.from_hash' do + it 'creates a PksOffense from a hash' do + offense = described_class.from_hash(privacy_offense_hash) + + expect(offense.violation_type).to eq('privacy') + expect(offense.file).to eq('packs/my_pack/app/models/user.rb') + expect(offense.line).to eq(42) + expect(offense.column).to eq(10) + expect(offense.constant_name).to eq('::OtherPack::PrivateClass') + expect(offense.referencing_pack_name).to eq('packs/my_pack') + expect(offense.defining_pack_name).to eq('packs/other_pack') + expect(offense.strict).to eq(false) + expect(offense.message).to eq('Privacy violation: ::OtherPack::PrivateClass is private') + end + + it 'defaults strict to false when not provided' do + hash = privacy_offense_hash.except('strict') + offense = described_class.from_hash(hash) + expect(offense.strict).to eq(false) + end + + it 'defaults message to empty string when not provided' do + hash = privacy_offense_hash.except('message') + offense = described_class.from_hash(hash) + expect(offense.message).to eq('') + end + end + + describe '.from_json' do + it 'parses JSON string with offenses array' do + json = { + 'offenses' => [privacy_offense_hash, dependency_offense_hash] + }.to_json + + offenses = described_class.from_json(json) + + expect(offenses.length).to eq(2) + expect(offenses[0].violation_type).to eq('privacy') + expect(offenses[1].violation_type).to eq('dependency') + end + + it 'returns empty array when no offenses key' do + json = {}.to_json + offenses = described_class.from_json(json) + expect(offenses).to eq([]) + end + + it 'returns empty array when offenses is empty' do + json = { 'offenses' => [] }.to_json + offenses = described_class.from_json(json) + expect(offenses).to eq([]) + end + end + + describe 'BasicReferenceOffense interface compatibility' do + let(:offense) { described_class.from_hash(privacy_offense_hash) } + + it 'provides class_name alias for constant_name' do + expect(offense.class_name).to eq(offense.constant_name) + end + + it 'provides type alias for violation_type' do + expect(offense.type).to eq(offense.violation_type) + end + + it 'provides to_package_name alias for defining_pack_name' do + expect(offense.to_package_name).to eq(offense.defining_pack_name) + end + + it 'provides from_package_name alias for referencing_pack_name' do + expect(offense.from_package_name).to eq(offense.referencing_pack_name) + end + end + + describe '#privacy?' do + it 'returns true for privacy violations' do + offense = described_class.from_hash(privacy_offense_hash) + expect(offense.privacy?).to eq(true) + expect(offense.dependency?).to eq(false) + end + end + + describe '#dependency?' do + it 'returns true for dependency violations' do + offense = described_class.from_hash(dependency_offense_hash) + expect(offense.dependency?).to eq(true) + expect(offense.privacy?).to eq(false) + end + end + + describe 'equality' do + let(:offense1) { described_class.from_hash(privacy_offense_hash) } + let(:offense2) { described_class.from_hash(privacy_offense_hash) } + let(:different_offense) { described_class.from_hash(dependency_offense_hash) } + + it 'considers offenses equal when key fields match' do + expect(offense1).to eq(offense2) + expect(offense1.eql?(offense2)).to eq(true) + end + + it 'considers offenses different when key fields differ' do + expect(offense1).not_to eq(different_offense) + expect(offense1.eql?(different_offense)).to eq(false) + end + + it 'produces consistent hash values for equal offenses' do + expect(offense1.hash).to eq(offense2.hash) + end + + it 'can be used in a Set' do + set = Set.new([offense1, offense2, different_offense]) + expect(set.length).to eq(2) + end + + it 'can be used as hash keys' do + hash = { offense1 => 'first', offense2 => 'second' } + expect(hash.length).to eq(1) + expect(hash[offense1]).to eq('second') + end + end + + describe 'fixture-based tests' do + describe 'no_violations.json' do + let(:json) { File.read(File.join(fixtures_path, 'no_violations.json')) } + + it 'parses empty offenses array' do + offenses = described_class.from_json(json) + expect(offenses).to eq([]) + end + end + + describe 'privacy_violation.json' do + let(:json) { File.read(File.join(fixtures_path, 'privacy_violation.json')) } + + it 'parses a single privacy violation' do + offenses = described_class.from_json(json) + + expect(offenses.length).to eq(1) + offense = offenses.first + expect(offense.violation_type).to eq('privacy') + expect(offense.file).to eq('packs/my_pack/app/models/user.rb') + expect(offense.line).to eq(42) + expect(offense.column).to eq(10) + expect(offense.constant_name).to eq('::OtherPack::PrivateClass') + expect(offense.referencing_pack_name).to eq('packs/my_pack') + expect(offense.defining_pack_name).to eq('packs/other_pack') + expect(offense.strict).to eq(false) + expect(offense.privacy?).to eq(true) + expect(offense.dependency?).to eq(false) + end + end + + describe 'dependency_violation.json' do + let(:json) { File.read(File.join(fixtures_path, 'dependency_violation.json')) } + + it 'parses a single dependency violation with strict mode' do + offenses = described_class.from_json(json) + + expect(offenses.length).to eq(1) + offense = offenses.first + expect(offense.violation_type).to eq('dependency') + expect(offense.file).to eq('packs/my_pack/app/services/user_service.rb') + expect(offense.line).to eq(15) + expect(offense.column).to eq(5) + expect(offense.constant_name).to eq('::ThirdPack::SomeConstant') + expect(offense.referencing_pack_name).to eq('packs/my_pack') + expect(offense.defining_pack_name).to eq('packs/third_pack') + expect(offense.strict).to eq(true) + expect(offense.privacy?).to eq(false) + expect(offense.dependency?).to eq(true) + end + end + + describe 'multiple_violations.json' do + let(:json) { File.read(File.join(fixtures_path, 'multiple_violations.json')) } + + it 'parses multiple violations of different types' do + offenses = described_class.from_json(json) + + expect(offenses.length).to eq(3) + + privacy_offenses = offenses.select(&:privacy?) + dependency_offenses = offenses.select(&:dependency?) + + expect(privacy_offenses.length).to eq(2) + expect(dependency_offenses.length).to eq(1) + end + + it 'preserves all offense details' do + offenses = described_class.from_json(json) + + files = offenses.map(&:file) + expect(files).to contain_exactly( + 'packs/my_pack/app/models/user.rb', + 'packs/my_pack/app/services/user_service.rb', + 'packs/another_pack/app/models/order.rb' + ) + end + end + + describe 'strict_mode_violation.json' do + let(:json) { File.read(File.join(fixtures_path, 'strict_mode_violation.json')) } + + it 'parses strict mode privacy violation' do + offenses = described_class.from_json(json) + + expect(offenses.length).to eq(1) + offense = offenses.first + expect(offense.strict).to eq(true) + expect(offense.privacy?).to eq(true) + end + end + end + end +end diff --git a/spec/danger_packwerk/pks_wrapper_spec.rb b/spec/danger_packwerk/pks_wrapper_spec.rb new file mode 100644 index 0000000..894c3f2 --- /dev/null +++ b/spec/danger_packwerk/pks_wrapper_spec.rb @@ -0,0 +1,128 @@ +require 'spec_helper' + +module DangerPackwerk + RSpec.describe PksWrapper do + let(:plugin) { dangerfile.packwerk } + + let(:valid_json_output) do + { + 'offenses' => [ + { + 'violation_type' => 'privacy', + 'file' => 'packs/my_pack/app/models/user.rb', + 'line' => 42, + 'column' => 10, + 'constant_name' => '::OtherPack::PrivateClass', + 'referencing_pack_name' => 'packs/my_pack', + 'defining_pack_name' => 'packs/other_pack', + 'strict' => false, + 'message' => 'Privacy violation' + } + ] + }.to_json + end + + let(:empty_json_output) do + { 'offenses' => [] }.to_json + end + + # Use real Process::Status from a successful command for Sorbet compatibility + let(:success_status) { `true`; $? } + let(:failure_status) { `false`; $? } + + describe '.get_offenses_for_files' do + it 'returns empty array when files is empty' do + expect(described_class.get_offenses_for_files([])).to eq([]) + end + + it 'calls pks check with JSON output format' do + allow(described_class).to receive(:run_pks_check) + .with(['file1.rb', 'file2.rb']) + .and_return([valid_json_output, '', success_status]) + + described_class.get_offenses_for_files(['file1.rb', 'file2.rb']) + + expect(described_class).to have_received(:run_pks_check).with(['file1.rb', 'file2.rb']) + end + + it 'returns PksOffense objects from JSON output' do + allow(described_class).to receive(:run_pks_check) + .and_return([valid_json_output, '', success_status]) + + offenses = described_class.get_offenses_for_files(['file1.rb']) + + expect(offenses.length).to eq(1) + expect(offenses.first).to be_a(PksOffense) + expect(offenses.first.violation_type).to eq('privacy') + expect(offenses.first.file).to eq('packs/my_pack/app/models/user.rb') + end + + it 'returns empty array when no violations found' do + allow(described_class).to receive(:run_pks_check) + .and_return([empty_json_output, '', success_status]) + + offenses = described_class.get_offenses_for_files(['file1.rb']) + + expect(offenses).to eq([]) + end + + it 'raises PksBinaryNotFoundError when pks command not found' do + allow(described_class).to receive(:run_pks_check) + .and_return(['', 'command not found: pks', failure_status]) + + expect do + described_class.get_offenses_for_files(['file1.rb']) + end.to raise_error(PksWrapper::PksBinaryNotFoundError, /pks binary not found/) + end + + it 'raises PksBinaryNotFoundError for No such file error' do + allow(described_class).to receive(:run_pks_check) + .and_return(['', 'No such file or directory', failure_status]) + + expect do + described_class.get_offenses_for_files(['file1.rb']) + end.to raise_error(PksWrapper::PksBinaryNotFoundError, /pks binary not found/) + end + end + + describe '.run_pks_check' do + it 'executes pks command with proper escaping' do + allow(Open3).to receive(:capture3) + .with('pks check --output-format json file1.rb file2.rb') + .and_return([empty_json_output, '', success_status]) + + described_class.run_pks_check(['file1.rb', 'file2.rb']) + + expect(Open3).to have_received(:capture3) + .with('pks check --output-format json file1.rb file2.rb') + end + + it 'escapes file paths with special characters' do + allow(Open3).to receive(:capture3).and_return([empty_json_output, '', success_status]) + + described_class.run_pks_check(['file with spaces.rb']) + + expect(Open3).to have_received(:capture3) + .with('pks check --output-format json file\\ with\\ spaces.rb') + end + end + + describe '.pks_available?' do + it 'returns true when pks is in PATH' do + allow(Open3).to receive(:capture3) + .with('which', 'pks') + .and_return(['/usr/local/bin/pks', '', success_status]) + + expect(described_class.pks_available?).to eq(true) + end + + it 'returns false when pks is not in PATH' do + allow(Open3).to receive(:capture3) + .with('which', 'pks') + .and_return(['', '', failure_status]) + + expect(described_class.pks_available?).to eq(false) + end + end + end +end diff --git a/spec/fixtures/pks_output/dependency_violation.json b/spec/fixtures/pks_output/dependency_violation.json new file mode 100644 index 0000000..d123f3d --- /dev/null +++ b/spec/fixtures/pks_output/dependency_violation.json @@ -0,0 +1,15 @@ +{ + "offenses": [ + { + "violation_type": "dependency", + "file": "packs/my_pack/app/services/user_service.rb", + "line": 15, + "column": 5, + "constant_name": "::ThirdPack::SomeConstant", + "referencing_pack_name": "packs/my_pack", + "defining_pack_name": "packs/third_pack", + "strict": true, + "message": "Dependency violation: packs/my_pack does not declare packs/third_pack as a dependency" + } + ] +} diff --git a/spec/fixtures/pks_output/multiple_violations.json b/spec/fixtures/pks_output/multiple_violations.json new file mode 100644 index 0000000..282fca8 --- /dev/null +++ b/spec/fixtures/pks_output/multiple_violations.json @@ -0,0 +1,37 @@ +{ + "offenses": [ + { + "violation_type": "privacy", + "file": "packs/my_pack/app/models/user.rb", + "line": 42, + "column": 10, + "constant_name": "::OtherPack::PrivateClass", + "referencing_pack_name": "packs/my_pack", + "defining_pack_name": "packs/other_pack", + "strict": false, + "message": "Privacy violation: ::OtherPack::PrivateClass is private to packs/other_pack" + }, + { + "violation_type": "dependency", + "file": "packs/my_pack/app/services/user_service.rb", + "line": 15, + "column": 5, + "constant_name": "::ThirdPack::SomeConstant", + "referencing_pack_name": "packs/my_pack", + "defining_pack_name": "packs/third_pack", + "strict": true, + "message": "Dependency violation: packs/my_pack does not declare packs/third_pack as a dependency" + }, + { + "violation_type": "privacy", + "file": "packs/another_pack/app/models/order.rb", + "line": 88, + "column": 15, + "constant_name": "::OtherPack::SecretHelper", + "referencing_pack_name": "packs/another_pack", + "defining_pack_name": "packs/other_pack", + "strict": false, + "message": "Privacy violation: ::OtherPack::SecretHelper is private to packs/other_pack" + } + ] +} diff --git a/spec/fixtures/pks_output/no_violations.json b/spec/fixtures/pks_output/no_violations.json new file mode 100644 index 0000000..4838c79 --- /dev/null +++ b/spec/fixtures/pks_output/no_violations.json @@ -0,0 +1,3 @@ +{ + "offenses": [] +} diff --git a/spec/fixtures/pks_output/privacy_violation.json b/spec/fixtures/pks_output/privacy_violation.json new file mode 100644 index 0000000..bd93a0c --- /dev/null +++ b/spec/fixtures/pks_output/privacy_violation.json @@ -0,0 +1,15 @@ +{ + "offenses": [ + { + "violation_type": "privacy", + "file": "packs/my_pack/app/models/user.rb", + "line": 42, + "column": 10, + "constant_name": "::OtherPack::PrivateClass", + "referencing_pack_name": "packs/my_pack", + "defining_pack_name": "packs/other_pack", + "strict": false, + "message": "Privacy violation: ::OtherPack::PrivateClass is private to packs/other_pack" + } + ] +} diff --git a/spec/fixtures/pks_output/strict_mode_violation.json b/spec/fixtures/pks_output/strict_mode_violation.json new file mode 100644 index 0000000..c4c4177 --- /dev/null +++ b/spec/fixtures/pks_output/strict_mode_violation.json @@ -0,0 +1,15 @@ +{ + "offenses": [ + { + "violation_type": "privacy", + "file": "packs/strict_pack/app/services/api_client.rb", + "line": 23, + "column": 8, + "constant_name": "::CorePack::InternalApi", + "referencing_pack_name": "packs/strict_pack", + "defining_pack_name": "packs/core_pack", + "strict": true, + "message": "Privacy violation (strict mode): ::CorePack::InternalApi is private to packs/core_pack" + } + ] +} diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 1cd31f7..7818448 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -74,3 +74,27 @@ def write_package_yml( ) write_pack(pack_name, { 'enforce_dependencies' => true, 'enforce_privacy' => true }) end + +def build_pks_offense( + violation_type:, + file:, + line:, + constant_name:, + referencing_pack_name:, + defining_pack_name:, + column: 5, + strict: false, + message: '' +) + DangerPackwerk::PksOffense.new( + violation_type: violation_type, + file: file, + line: line, + column: column, + constant_name: constant_name, + referencing_pack_name: referencing_pack_name, + defining_pack_name: defining_pack_name, + strict: strict, + message: message + ) +end