diff --git a/README.md b/README.md index 78d692b..b405686 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # danger-packwerk -`danger-packwerk` integrates [`packwerk`](https://github.com/Shopify/packwerk) with [`danger`](https://github.com/danger/danger) to provide inline comments in PRs related to boundaries in a Rails application. +`danger-packwerk` integrates [`packwerk`](https://github.com/Shopify/packwerk) (via [`pks`](https://github.com/rubyatscale/pks)) with [`danger`](https://github.com/danger/danger) to provide inline comments in PRs related to boundaries in a Rails application. ## Installation and Basic Usage Step 1: Add this line to your `Gemfile` (to whatever group your CI uses, as it is not needed in production) and `bundle install`: @@ -18,10 +18,108 @@ package_todo_yml_changes.check That's it for basic usage! +## pks Integration + +`danger-packwerk` uses [pks](https://github.com/rubyatscale/pks), a fast Rust implementation of packwerk's boundary checking. This provides significant performance improvements, especially for large codebases. + +### Why pks? + +- **Speed**: pks is written in Rust and runs 10-100x faster than the Ruby packwerk implementation +- **Compatibility**: pks produces the same violation types (dependency, privacy) as packwerk +- **JSON Output**: Native JSON output format for reliable parsing + +### Installing pks + +You have two options for installing pks: + +**Option 1: Using dotslash (recommended)** + +[dotslash](https://dotslash-cli.com/) manages tool versions declaratively. Create a `pks` file in your project: + +```json +#!/usr/bin/env dotslash + +{ + "name": "pks", + "platforms": { + "macos-aarch64": { + "size": 3456789, + "hash": "sha256:...", + "url": "https://github.com/alexcrichton/pks/releases/download/v0.1.0/pks-aarch64-apple-darwin.tar.gz", + "path": "pks" + }, + "linux-x86_64": { + "size": 4567890, + "hash": "sha256:...", + "url": "https://github.com/alexcrichton/pks/releases/download/v0.1.0/pks-x86_64-unknown-linux-gnu.tar.gz", + "path": "pks" + } + } +} +``` + +Then ensure it's in your PATH or use `./pks` directly. + +**Option 2: Using cargo** + +If you have Rust installed: + +```bash +cargo install pks +``` + +### Migration from packwerk + +No code changes are required. `danger-packwerk` automatically uses pks when calling `packwerk.check`. The integration is transparent—your existing `Dangerfile` configuration continues to work. + +If you were previously relying on `bin/packwerk check` being called directly, note that danger-packwerk now uses `pks check --output-format json` internally. + +### JSON Output Format + +pks outputs violations in a structured JSON format that danger-packwerk parses: + +```json +{ + "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: ..." + } + ] +} +``` + +This format provides precise line and column information for accurate inline comments. + +### Error Handling + +If pks is not installed or not found in PATH, danger-packwerk raises `DangerPackwerk::PksWrapper::PksBinaryNotFoundError` with the message: + +``` +pks binary not found. Please install pks to use this feature. +``` + +To check if pks is available before running: + +```ruby +if DangerPackwerk::PksWrapper.pks_available? + packwerk.check +else + warn "pks not installed, skipping packwerk check" +end +``` + ## Advanced Usage There are currently two danger checks that ship with `danger-packwerk`: -1) One that runs `bin/packwerk check` and leaves inline comments in source code on new violations +1) One that runs `pks check` and leaves inline comments in source code on new violations 2) One that looks at changes to `package_todo.yml` files and leaves inline comments on added violations. In upcoming iterations, we will include other danger checks, including: 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/check/default_formatter.rb b/lib/danger-packwerk/check/default_formatter.rb index da8542e..484d393 100644 --- a/lib/danger-packwerk/check/default_formatter.rb +++ b/lib/danger-packwerk/check/default_formatter.rb @@ -19,23 +19,23 @@ def initialize(custom_help_message: nil) sig do override.params( - offenses: T::Array[Packwerk::ReferenceOffense], + offenses: T::Array[PksOffense], repo_link: String, org_name: String, repo_url_builder: T.nilable(T.proc.params(constant_path: String).returns(String)) ).returns(String) end def format_offenses(offenses, repo_link, org_name, repo_url_builder: nil) - reference_offense = T.must(offenses.first) + offense = T.must(offenses.first) violation_types = offenses.map(&:violation_type) - referencing_file = reference_offense.reference.relative_path + referencing_file = offense.file referencing_file_pack = ParsePackwerk.package_from_path(referencing_file).name # We remove leading double colons as they feel like an implementation detail of packwerk. - constant_name = reference_offense.reference.constant.name.delete_prefix('::') + constant_name = offense.constant_name.delete_prefix('::') - constant_source_package_name = reference_offense.reference.constant.package.name + constant_source_package_name = offense.defining_pack_name - constant_location = reference_offense.reference.constant.location + constant_location = Private.constant_resolver.resolve(offense.constant_name)&.location || offense.file constant_source_package = T.must(ParsePackwerk.all.find { |p| p.name == constant_source_package_name }) constant_source_package_ownership_info = Private::OwnershipInformation.for_package(constant_source_package, org_name) diff --git a/lib/danger-packwerk/check/offenses_formatter.rb b/lib/danger-packwerk/check/offenses_formatter.rb index 637f443..6da3f4b 100644 --- a/lib/danger-packwerk/check/offenses_formatter.rb +++ b/lib/danger-packwerk/check/offenses_formatter.rb @@ -12,7 +12,7 @@ module OffensesFormatter sig do abstract.params( - offenses: T::Array[Packwerk::ReferenceOffense], + offenses: T::Array[PksOffense], repo_link: String, org_name: String, repo_url_builder: T.nilable(T.proc.params(constant_path: String).returns(String)) diff --git a/lib/danger-packwerk/danger_packwerk.rb b/lib/danger-packwerk/danger_packwerk.rb index af3a472..383dbff 100644 --- a/lib/danger-packwerk/danger_packwerk.rb +++ b/lib/danger-packwerk/danger_packwerk.rb @@ -5,7 +5,8 @@ require 'packwerk' 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 +19,7 @@ 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 } + OnFailure = T.type_alias { T.proc.params(offenses: T::Array[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 +102,53 @@ 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 - filepath_that_defines_this_constant = Private.constant_resolver.resolve(constant_name)&.location + offenses_to_care_about = pks_offenses.reject do |offense| + filepath_that_defines_this_constant = Private.constant_resolver.resolve(offense.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.constant_name, + offense.line, + offense.file ] when CommentGroupingStrategy::PerConstantPerPack [ - packwerk_reference_offense.reference.constant.name, - ParsePackwerk.package_from_path(packwerk_reference_offense.reference.relative_path) + offense.constant_name, + ParsePackwerk.package_from_path(offense.file) ] 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.line + referencing_file = offense.file - message = offenses_formatter.format_offenses(unique_packwerk_reference_offenses, repo_link, org_name, repo_url_builder: repo_url_builder) + message = offenses_formatter.format_offenses(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/packwerk_wrapper.rb b/lib/danger-packwerk/packwerk_wrapper.rb deleted file mode 100644 index 9484fc8..0000000 --- a/lib/danger-packwerk/packwerk_wrapper.rb +++ /dev/null @@ -1,73 +0,0 @@ -# typed: strict - -require 'stringio' - -module DangerPackwerk - # This class wraps packwerk to give us precisely what we want, which is the `Packwerk::ReferenceOffense` from a set of files. - # Note that statically packwerk returns `Packwerk::Offense` from running `bin/packwerk check`. The two types of `Packwerk::Offense` are - # `Packwerk::ReferenceOffense` and `Packwerk::Parsers::ParseResult`.`Packwerk::ReferenceOffense` inherits from `Packwerk::Offense`, and has more info than `Packwerk::Offense`. - # `Packwerk::Parsers::ParseResult` is returned when there is a file parsing issue. We ignore ParseResult types as it's likely that other tests would break along with this Danger check. - # and it is not the intent of this check to look for syntax errors in code. - # - # Also note that we would not need most of this class if there were two changes made to Packwerk: - # 1) It did not raise if no checkable files were found (I think it might make more sense to just return successfully rather than raise). This occurs if the - # input file list is excluded from the user's `exclude` list. In this case, check should return that no errors were found, since those files were not analyzed. - # 2) If the CLI gave a way to get offenses from files without this somewhat hacky way of passing in a formatter that stores the offenses. - class PackwerkWrapper - extend T::Sig - - # This code is partially copied from exe/packwerk within the packwerk gem. We're imitating the - # cli here but with our own offense formatter to collect the violating data directly. - # - # We capture and ignore the output of the Cli so that we don't leak it to the build system logs. - # When packwerk produces errors it can make the build system look like it's failing when really - # this is expected behavior. - sig { params(files: T::Array[String]).returns(T::Array[Packwerk::ReferenceOffense]) } - def self.get_offenses_for_files(files) - formatter = OffensesAggregatorFormatter.new - ENV['RAILS_ENV'] = 'test' - cli = Packwerk::Cli.new(offenses_formatter: formatter, out: StringIO.new) - cli.execute_command(['check', *files]) - reference_offenses = formatter.aggregated_offenses.compact.select { |offense| offense.is_a?(Packwerk::ReferenceOffense) } - T.cast(reference_offenses, T::Array[Packwerk::ReferenceOffense]) - end - - # - # This Packwerk formatter simply collects offenses. Ideally we could accomplish this by calling into public API of the CLI, - # but right now this is the only way to get the raw offenses out of packwerk. - # - class OffensesAggregatorFormatter - extend T::Sig - include Packwerk::OffensesFormatter - - sig { returns(T::Array[Packwerk::Offense]) } - attr_reader :aggregated_offenses - - sig { void } - def initialize - @aggregated_offenses = T.let([], T::Array[Packwerk::ReferenceOffense]) - end - - sig { override.params(offenses: T::Array[T.nilable(Packwerk::Offense)]).returns(String) } - def show_offenses(offenses) - @aggregated_offenses = T.unsafe(offenses) - '' - end - - sig { override.params(offense_collection: Packwerk::OffenseCollection, for_files: T::Set[String]).returns(String) } - def show_stale_violations(offense_collection, for_files) - '' - end - - sig { override.params(strict_mode_violations: T::Array[::Packwerk::ReferenceOffense]).returns(::String) } - def show_strict_mode_violations(strict_mode_violations) - '' - end - - sig { override.returns(::String) } - def identifier - 'danger_packwerk_offenses_aggregator' - end - 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..9f9ba79 --- /dev/null +++ b/lib/danger-packwerk/pks_offense.rb @@ -0,0 +1,98 @@ +# typed: strict + +require 'json' + +module DangerPackwerk + # + # PksOffense represents a violation from pks JSON output. + # It has a compatible interface with BasicReferenceOffense for use with formatters. + # + 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 + + # 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..c01a5f1 --- /dev/null +++ b/lib/danger-packwerk/pks_wrapper.rb @@ -0,0 +1,37 @@ +# 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) + + raise PksBinaryNotFoundError, 'pks binary not found. Please install pks to use this feature.' if stderr.include?('command not found') || stderr.include?('No such file or directory') + + 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..a0d2a00 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 } @@ -71,10 +57,11 @@ module DangerPackwerk include Check::OffensesFormatter def format_offenses(offenses, repo_link, org_name, repo_url_builder:) - constant_location = offenses.first.reference.constant.location - constant_name = offenses.first.reference.constant.name + offense = offenses.first + constant_location = offense.file + constant_name = offense.constant_name url = repo_url_builder&.call(constant_location.to_s) - offenses.map { |offense| "#{offense.message} [#{constant_name}](#{url})" }.join("\n\n") + offenses.map { |o| "#{o.message} [#{constant_name}](#{url})" }.join("\n\n") end end end @@ -93,12 +80,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 @@ -109,11 +95,25 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:) end end + context 'when pks binary is not found' do + before do + allow(PksWrapper).to receive(:get_offenses_for_files) + .and_raise(PksWrapper::PksBinaryNotFoundError, 'pks binary not found. Please install pks to use this feature.') + end + + it 'raises PksBinaryNotFoundError with helpful message' do + expect { packwerk_check }.to raise_error( + PksWrapper::PksBinaryNotFoundError, + /pks binary not found.*install pks/ + ) + end + end + context 'when the only files modified are ones that packwerk ignores' do 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 +133,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 +154,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 +174,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 +194,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 +202,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 +227,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 +235,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 +261,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 +273,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 +306,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 +317,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 +352,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 +360,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 +371,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 +404,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 +412,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 +423,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 +456,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 +464,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 +482,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 +515,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 +526,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 +559,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 +572,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 +602,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 offense.file 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 +628,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 +661,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 +669,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 +682,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 +780,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 +805,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 +833,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 +865,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 +890,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 +932,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 +942,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 +973,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 +983,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 +1016,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 +1026,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 +1057,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 +1067,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 +1091,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 +1126,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 +1136,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 +1157,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..6bf5e9d --- /dev/null +++ b/spec/danger_packwerk/pks_wrapper_spec.rb @@ -0,0 +1,135 @@ +require 'spec_helper' +require 'English' + +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) do + `true` + $CHILD_STATUS + end + let(:failure_status) do + `false` + $CHILD_STATUS + end + + 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