Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve RSpec/VerifiedDoubles todos #14400

Merged
merged 20 commits into from Jan 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions Library/Homebrew/dependable.rb
Expand Up @@ -11,6 +11,8 @@ module Dependable
# misuse in future.
RESERVED_TAGS = [:build, :optional, :recommended, :run, :test, :linked].freeze

attr_reader :tags
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

This is a bit tangential, but moving the attr here reduces duplication, allows us to remove the matching .rbi file, and write better tests.


def build?
tags.include? :build
end
Expand Down
5 changes: 0 additions & 5 deletions Library/Homebrew/dependable.rbi

This file was deleted.

2 changes: 1 addition & 1 deletion Library/Homebrew/dependency.rb
Expand Up @@ -13,7 +13,7 @@ class Dependency
include Dependable
extend Cachable

attr_reader :name, :tags, :env_proc, :option_names
attr_reader :name, :env_proc, :option_names

DEFAULT_ENV_PROC = proc {}.freeze
private_constant :DEFAULT_ENV_PROC
Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/requirement.rb
Expand Up @@ -17,7 +17,7 @@ class Requirement
include Dependable
extend Cachable

attr_reader :tags, :name, :cask, :download
attr_reader :name, :cask, :download

def initialize(tags = [])
# Only allow instances of subclasses. This base class enforces no constraints on its own.
Expand Down
24 changes: 0 additions & 24 deletions Library/Homebrew/test/.rubocop_todo.yml
Expand Up @@ -13,27 +13,3 @@ RSpec/InstanceVariable:
- "download_strategies/git_spec.rb"
- "support/helper/spec/shared_context/integration_test.rb"
- "utils/git_spec.rb"

# Offense count: 63
# Configuration parameters: IgnoreNameless, IgnoreSymbolicNames.
RSpec/VerifiedDoubles:
Exclude:
- "cache_store_spec.rb"
- "cask/artifact/pkg_spec.rb"
- "cask/installer_spec.rb"
- "cask/pkg_spec.rb"
- "cmd/update-report_spec.rb"
- "compiler_failure_spec.rb"
- "compiler_selector_spec.rb"
- "dependable_spec.rb"
- "dependency_expansion_spec.rb"
- "description_cache_store_spec.rb"
- "exceptions_spec.rb"
- "formula_pin_spec.rb"
- "formula_spec.rb"
- "language/python/virtualenv_spec.rb"
- "linkage_cache_store_spec.rb"
- "resource_spec.rb"
- "software_spec_spec.rb"
- "utils/analytics_spec.rb"
- "version_spec.rb"
12 changes: 6 additions & 6 deletions Library/Homebrew/test/cache_store_spec.rb
Expand Up @@ -10,15 +10,15 @@
let(:type) { :test }

it "creates a new `DatabaseCache` instance" do
cache_store = double("cache_store", write_if_dirty!: nil)
cache_store = instance_double(described_class, "cache_store", write_if_dirty!: nil)
expect(described_class).to receive(:new).with(type).and_return(cache_store)
expect(cache_store).to receive(:write_if_dirty!)
described_class.use(type) { |_db| }
end
end

describe "#set" do
let(:db) { double("db", :[]= => nil) }
let(:db) { instance_double(Hash, "db", :[]= => nil) }

it "sets the value in the `CacheStoreDatabase`" do
allow(File).to receive(:write)
Expand All @@ -33,7 +33,7 @@

describe "#get" do
context "with a database created" do
let(:db) { double("db", :[] => "bar") }
let(:db) { instance_double(Hash, "db", :[] => "bar") }

it "gets value in the `CacheStoreDatabase` corresponding to the key" do
allow(sample_db).to receive(:created?).and_return(true)
Expand All @@ -45,7 +45,7 @@
end

context "without a database created" do
let(:db) { double("db", :[] => nil) }
let(:db) { instance_double(Hash, "db", :[] => nil) }

before do
allow(sample_db).to receive(:created?).and_return(false)
Expand All @@ -65,7 +65,7 @@

describe "#delete" do
context "with a database created" do
let(:db) { double("db", :[] => { foo: "bar" }) }
let(:db) { instance_double(Hash, "db", :[] => { foo: "bar" }) }

before do
allow(sample_db).to receive(:created?).and_return(true)
Expand All @@ -79,7 +79,7 @@
end

context "without a database created" do
let(:db) { double("db", delete: nil) }
let(:db) { instance_double(Hash, "db", delete: nil) }

before do
allow(sample_db).to receive(:created?).and_return(false)
Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/test/cask/artifact/pkg_spec.rb
Expand Up @@ -35,7 +35,7 @@
it "passes the choice changes xml to the system installer" do
pkg = cask.artifacts.find { |a| a.is_a?(described_class) }

file = double(path: Pathname.new("/tmp/choices.xml"))
file = instance_double(Tempfile, path: Pathname.new("/tmp/choices.xml"))

expect(file).to receive(:write).with <<~XML
<?xml version="1.0" encoding="UTF-8"?>
Expand Down
4 changes: 0 additions & 4 deletions Library/Homebrew/test/cask/installer_spec.rb
Expand Up @@ -3,10 +3,6 @@

describe Cask::Installer, :cask do
describe "install" do
let(:empty_depends_on_stub) {
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Appears to be unused (the last reference was removed in 7d5b8a5cea122c09f297dffa6b76187c413a9565)

double(formula: [], cask: [], macos: nil, arch: nil)
}

it "downloads and installs a nice fresh Cask" do
caffeine = Cask::CaskLoader.load(cask_path("local-caffeine"))

Expand Down
8 changes: 7 additions & 1 deletion Library/Homebrew/test/cask/pkg_spec.rb
Expand Up @@ -4,7 +4,13 @@
describe Cask::Pkg, :cask do
describe "#uninstall" do
let(:fake_system_command) { NeverSudoSystemCommand }
let(:empty_response) { double(stdout: "", plist: { "volume" => "/", "install-location" => "", "paths" => {} }) }
let(:empty_response) do
instance_double(
SystemCommand::Result,
stdout: "",
plist: { "volume" => "/", "install-location" => "", "paths" => {} },
)
end
let(:pkg) { described_class.new("my.fake.pkg", fake_system_command) }

it "removes files and dirs referenced by the pkg" do
Expand Down
4 changes: 2 additions & 2 deletions Library/Homebrew/test/cmd/update-report_spec.rb
Expand Up @@ -27,8 +27,8 @@ def initialize(tap)
let(:hub) { ReporterHub.new }

def perform_update(fixture_name = "")
allow(Formulary).to receive(:factory).and_return(double(pkg_version: "1.0"))
allow(FormulaVersions).to receive(:new).and_return(double(formula_at_revision: "2.0"))
allow(Formulary).to receive(:factory).and_return(instance_double(Formula, pkg_version: "1.0"))
allow(FormulaVersions).to receive(:new).and_return(instance_double(FormulaVersions, formula_at_revision: "2.0"))

diff = YAML.load_file("#{TEST_FIXTURE_DIR}/updater_fixture.yaml")[fixture_name]
allow(reporter).to receive(:diff).and_return(diff || "")
Expand Down
44 changes: 35 additions & 9 deletions Library/Homebrew/test/compiler_failure_spec.rb
Expand Up @@ -9,31 +9,57 @@
describe "::create" do
it "creates a failure when given a symbol" do
failure = described_class.create(:clang)
expect(failure).to fail_with(double("Compiler", type: :clang, name: :clang, version: 600))
expect(failure).to fail_with(
instance_double(CompilerSelector::Compiler, "Compiler", type: :clang, name: :clang, version: 600),
)
end

it "can be given a build number in a block" do
failure = described_class.create(:clang) { build 700 }
expect(failure).to fail_with(double("Compiler", type: :clang, name: :clang, version: 700))
expect(failure).to fail_with(
instance_double(CompilerSelector::Compiler, "Compiler", type: :clang, name: :clang, version: 700),
)
end

it "can be given an empty block" do
failure = described_class.create(:clang) {}
expect(failure).to fail_with(double("Compiler", type: :clang, name: :clang, version: 600))
expect(failure).to fail_with(
instance_double(CompilerSelector::Compiler, "Compiler", type: :clang, name: :clang, version: 600),
)
end

it "creates a failure when given a hash" do
failure = described_class.create(gcc: "7")
expect(failure).to fail_with(double("Compiler", type: :gcc, name: "gcc-7", version: Version.new("7")))
expect(failure).to fail_with(double("Compiler", type: :gcc, name: "gcc-7", version: Version.new("7.1")))
expect(failure).not_to fail_with(double("Compiler", type: :gcc, name: "gcc-6", version: Version.new("6.0")))
expect(failure).to fail_with(
instance_double(CompilerSelector::Compiler, "Compiler", type: :gcc, name: "gcc-7", version: Version.new("7")),
)
expect(failure).to fail_with(
instance_double(
CompilerSelector::Compiler, "Compiler", type: :gcc, name: "gcc-7", version: Version.new("7.1")
),
)
expect(failure).not_to fail_with(
instance_double(
CompilerSelector::Compiler, "Compiler", type: :gcc, name: "gcc-6", version: Version.new("6.0")
),
)
end

it "creates a failure when given a hash and a block with aversion" do
failure = described_class.create(gcc: "7") { version "7.1" }
expect(failure).to fail_with(double("Compiler", type: :gcc, name: "gcc-7", version: Version.new("7")))
expect(failure).to fail_with(double("Compiler", type: :gcc, name: "gcc-7", version: Version.new("7.1")))
expect(failure).not_to fail_with(double("Compiler", type: :gcc, name: "gcc-7", version: Version.new("7.2")))
expect(failure).to fail_with(
instance_double(CompilerSelector::Compiler, "Compiler", type: :gcc, name: "gcc-7", version: Version.new("7")),
)
expect(failure).to fail_with(
instance_double(
CompilerSelector::Compiler, "Compiler", type: :gcc, name: "gcc-7", version: Version.new("7.1")
),
)
expect(failure).not_to fail_with(
instance_double(
CompilerSelector::Compiler, "Compiler", type: :gcc, name: "gcc-7", version: Version.new("7.2")
),
)
end
end
end
19 changes: 9 additions & 10 deletions Library/Homebrew/test/compiler_selector_spec.rb
Expand Up @@ -10,12 +10,7 @@
let(:compilers) { [:clang, :gnu] }
let(:software_spec) { SoftwareSpec.new }
let(:cc) { :clang }
let(:versions) do
double(
llvm_build_version: Version::NULL,
clang_build_version: Version.create("600"),
)
end
let(:versions) { class_double(DevelopmentTools, clang_build_version: Version.create("600")) }
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I think this is the correct class_double, but it doesn't recognize llvm_build_version


before do
allow(versions).to receive(:gcc_version) do |name|
Expand Down Expand Up @@ -46,28 +41,32 @@

it "returns gcc-6 if gcc formula offers gcc-6 on mac", :needs_macos do
software_spec.fails_with(:clang)
allow(Formulary).to receive(:factory).with("gcc").and_return(double(version: Version.new("6.0")))
allow(Formulary).to receive(:factory).with("gcc")
.and_return(instance_double(Formula, version: Version.new("6.0")))
expect(selector.compiler).to eq("gcc-6")
end

it "returns gcc-5 if gcc formula offers gcc-5 on linux", :needs_linux do
software_spec.fails_with(:clang)
allow(Formulary).to receive(:factory).with("gcc@11").and_return(double(version: Version.new("5.0")))
allow(Formulary).to receive(:factory).with("gcc@11")
.and_return(instance_double(Formula, version: Version.new("5.0")))
expect(selector.compiler).to eq("gcc-5")
end

it "returns gcc-6 if gcc formula offers gcc-5 and fails with gcc-5 and gcc-7 on linux", :needs_linux do
software_spec.fails_with(:clang)
software_spec.fails_with(gcc: "5")
software_spec.fails_with(gcc: "7")
allow(Formulary).to receive(:factory).with("gcc@11").and_return(double(version: Version.new("5.0")))
allow(Formulary).to receive(:factory).with("gcc@11")
.and_return(instance_double(Formula, version: Version.new("5.0")))
expect(selector.compiler).to eq("gcc-6")
end

it "returns gcc-7 if gcc formula offers gcc-5 and fails with gcc <= 6 on linux", :needs_linux do
software_spec.fails_with(:clang)
software_spec.fails_with(:gcc) { version "6" }
allow(Formulary).to receive(:factory).with("gcc@11").and_return(double(version: Version.new("5.0")))
allow(Formulary).to receive(:factory).with("gcc@11")
.and_return(instance_double(Formula, version: Version.new("5.0")))
expect(selector.compiler).to eq("gcc-7")
end

Expand Down
11 changes: 8 additions & 3 deletions Library/Homebrew/test/dependable_spec.rb
Expand Up @@ -6,9 +6,14 @@
describe Dependable do
alias_matcher :be_a_build_dependency, :be_build

subject(:dependable) { double(tags: tags).extend(described_class) }

let(:tags) { ["foo", "bar", :build] }
subject(:dependable) {
Class.new {
include Dependable
def initialize
@tags = ["foo", "bar", :build]
end
}.new
}

specify "#options" do
expect(dependable.options.as_flags.sort).to eq(%w[--foo --bar].sort)
Expand Down
23 changes: 12 additions & 11 deletions Library/Homebrew/test/dependency_expansion_spec.rb
Expand Up @@ -6,7 +6,7 @@
describe Dependency do
def build_dep(name, tags = [], deps = [])
dep = described_class.new(name.to_s, tags)
allow(dep).to receive(:to_formula).and_return(double(deps: deps, name: name))
allow(dep).to receive(:to_formula).and_return(instance_double(Formula, deps: deps, name: name))
dep
end

Expand All @@ -15,7 +15,7 @@ def build_dep(name, tags = [], deps = [])
let(:baz) { build_dep(:baz) }
let(:qux) { build_dep(:qux) }
let(:deps) { [foo, bar, baz, qux] }
let(:formula) { double(deps: deps, name: "f") }
let(:formula) { instance_double(Formula, deps: deps, name: "f") }

describe "::expand" do
it "yields dependent and dependency pairs" do
Expand Down Expand Up @@ -44,20 +44,20 @@ def build_dep(name, tags = [], deps = [])
end

it "preserves dependency order" do
allow(foo).to receive(:to_formula).and_return(double(name: "f", deps: [qux, baz]))
allow(foo).to receive(:to_formula).and_return(instance_double(Formula, name: "f", deps: [qux, baz]))
expect(described_class.expand(formula)).to eq([qux, baz, foo, bar])
end
end

it "skips optionals by default" do
deps = [build_dep(:foo, [:optional]), bar, baz, qux]
f = double(deps: deps, build: double(with?: false), name: "f")
f = instance_double(Formula, deps: deps, build: instance_double(BuildOptions, with?: false), name: "f")
expect(described_class.expand(f)).to eq([bar, baz, qux])
end

it "keeps recommended dependencies by default" do
deps = [build_dep(:foo, [:recommended]), bar, baz, qux]
f = double(deps: deps, build: double(with?: true), name: "f")
f = instance_double(Formula, deps: deps, build: instance_double(BuildOptions, with?: true), name: "f")
expect(described_class.expand(f)).to eq(deps)
end

Expand All @@ -75,7 +75,7 @@ def build_dep(name, tags = [], deps = [])
it "merges dependencies and preserves env_proc" do
env_proc = double
dep = described_class.new("foo", [], env_proc)
allow(dep).to receive(:to_formula).and_return(double(deps: [], name: "foo"))
allow(dep).to receive(:to_formula).and_return(instance_double(Formula, deps: [], name: "foo"))
deps.replace([dep])
expect(described_class.expand(formula).first.env_proc).to eq(env_proc)
end
Expand All @@ -89,7 +89,8 @@ def build_dep(name, tags = [], deps = [])
end

it "skips parent but yields children with ::skip" do
f = double(
f = instance_double(
Formula,
name: "f",
deps: [
build_dep(:foo, [], [bar, baz]),
Expand All @@ -107,7 +108,7 @@ def build_dep(name, tags = [], deps = [])
it "keeps dependency but prunes recursive dependencies with ::keep_but_prune_recursive_deps" do
foo = build_dep(:foo, [:test], bar)
baz = build_dep(:baz, [:test])
f = double(name: "f", deps: [foo, baz])
f = instance_double(Formula, name: "f", deps: [foo, baz])

deps = described_class.expand(f) do |_dependent, dep|
described_class.keep_but_prune_recursive_deps if dep.test?
Expand All @@ -124,15 +125,15 @@ def build_dep(name, tags = [], deps = [])
it "doesn't raise an error when a dependency is cyclic" do
foo = build_dep(:foo)
bar = build_dep(:bar, [], [foo])
allow(foo).to receive(:to_formula).and_return(double(deps: [bar], name: foo.name))
f = double(name: "f", deps: [foo, bar])
allow(foo).to receive(:to_formula).and_return(instance_double(Formula, deps: [bar], name: foo.name))
f = instance_double(Formula, name: "f", deps: [foo, bar])
expect { described_class.expand(f) }.not_to raise_error
end

it "cleans the expand stack" do
foo = build_dep(:foo)
allow(foo).to receive(:to_formula).and_raise(FormulaUnavailableError, foo.name)
f = double(name: "f", deps: [foo])
f = instance_double(Formula, name: "f", deps: [foo])
expect { described_class.expand(f) }.to raise_error(FormulaUnavailableError)
expect(described_class.instance_variable_get(:@expand_stack)).to be_empty
end
Expand Down