diff --git a/docs/reference.md b/docs/reference.md index d9eb4fd..6f37eb9 100644 --- a/docs/reference.md +++ b/docs/reference.md @@ -280,7 +280,7 @@ Subclass `FixtureKit::Adapter` and implement: `#identifier_for(identifier)` - Receives non-string fixture identifier and returns normalized String identifier. -- `_anonymous/` prefixing is applied by `FixtureKit::Cache`. +- `_anonymous/` prefixing and the declaration-site digest suffix are applied by `FixtureKit::Cache`. Adapter initialization: @@ -332,12 +332,21 @@ Cache file path format: Identifier behavior: - Named fixture: identifier is the fixture name string. -- Anonymous fixture: identifier is `_anonymous/`. +- Anonymous fixture: identifier is `_anonymous/.`. + +The digest is the first 12 hex characters of a SHA-256 of the declaration site +(the `fixture` block's file and line, plus its `extends:` target). The scope name +alone is not unique: test frameworks derive it from the group description, which +drops non-alphanumeric characters, and test runners that load specs in batches +call `RSpec::Core::World#reset` between them, which resets the counter RSpec uses +to disambiguate duplicate descriptions. Without the digest, two spec files +sharing a top-level description would share one cache entry, and the second +fixture to generate would mount the first one's records. Examples: - Named: `teams/basic` -> `tmp/cache/fixture_kit/teams/basic.json` -- Anonymous RSpec: `_anonymous/foo/with_fixture_kit/hello` -- Anonymous Minitest: `_anonymous/my_feature_test` +- Anonymous RSpec: `_anonymous/foo/with_fixture_kit/hello.3f2a9c1d4b8e` +- Anonymous Minitest: `_anonymous/my_feature_test.7c1b0e5a9d24` ## Runtime API in Tests diff --git a/lib/fixture_kit.rb b/lib/fixture_kit.rb index a718648..f0e810d 100644 --- a/lib/fixture_kit.rb +++ b/lib/fixture_kit.rb @@ -6,6 +6,7 @@ class DuplicateNameError < Error; end class InvalidFixtureDeclaration < Error; end class MultipleFixtures < Error; end class CacheMissingError < Error; end + class CacheIdentifierCollision < Error; end class FixtureDefinitionNotFound < Error; end class RunnerAlreadyStartedError < Error; end class CircularFixtureInheritance < Error; end diff --git a/lib/fixture_kit/cache.rb b/lib/fixture_kit/cache.rb index 34ba0c9..2214890 100644 --- a/lib/fixture_kit/cache.rb +++ b/lib/fixture_kit/cache.rb @@ -1,8 +1,11 @@ # frozen_string_literal: true +require "digest" + module FixtureKit class Cache ANONYMOUS_DIRECTORY = "_anonymous" + DIGEST_LENGTH = 12 include ConfigurationHelper @@ -22,7 +25,8 @@ def identifier if raw_identifier.is_a?(String) raw_identifier else - File.join(ANONYMOUS_DIRECTORY, FixtureKit.runner.adapter.identifier_for(raw_identifier)) + normalized_scope = FixtureKit.runner.adapter.identifier_for(raw_identifier) + File.join(ANONYMOUS_DIRECTORY, "#{normalized_scope}.#{definition_digest}") end end end @@ -62,6 +66,26 @@ def save private + # The scope name alone does not identify a declaration site. Test + # frameworks derive it from the group description, which is lossy -- RSpec + # strips every non-alphanumeric character, so `Foo::Bar` and `"Foo Bar"` + # both become `FooBar` -- and, worse, not even unique within a process: + # test runners that load specs in batches call RSpec::Core::World#reset + # between them, which drops the constants RSpec disambiguates against, so + # the first group of a given description in *every* batch takes the + # unsuffixed name. The cache directory is cleared once per process, so + # without the digest two spec files sharing a top-level description share + # this entry, and the second fixture to generate silently mounts the + # first's records. + # + # Joined to the scope with a dot rather than an underscore: scope names are + # snake_case, so an underscore leaves no boundary between the two, while a + # dot cannot appear in one -- RSpec strips non-alphanumerics from the group + # description before it is underscored. + def definition_digest + Digest::SHA256.hexdigest(fixture.definition.fingerprint)[0, DIGEST_LENGTH] + end + def evaluate(coders, context, data = {}, &block) if coders.empty? fixture.definition.evaluate(context, parent: fixture.parent&.mount) diff --git a/lib/fixture_kit/definition.rb b/lib/fixture_kit/definition.rb index e646551..7f58c64 100644 --- a/lib/fixture_kit/definition.rb +++ b/lib/fixture_kit/definition.rb @@ -14,6 +14,17 @@ def path @definition.source_location.first end + def location + file, line = @definition.source_location + "#{file}:#{line}" + end + + # Two definitions sharing a fingerprint evaluate the same block against the + # same parent, so they are interchangeable and may share a cache entry. + def fingerprint + "#{location}:#{extends}" + end + def evaluate(context, parent: nil) context.singleton_class.prepend(mixin(parent)) context.instance_exec(&@definition) diff --git a/lib/fixture_kit/fixture.rb b/lib/fixture_kit/fixture.rb index 151cbb6..1c83d5d 100644 --- a/lib/fixture_kit/fixture.rb +++ b/lib/fixture_kit/fixture.rb @@ -19,6 +19,8 @@ def initialize(identifier, definition) end def generate(force: false) + FixtureKit.runner.registry.claim_cache_identifier(self) + return if @cache.exists? && !force parent&.generate diff --git a/lib/fixture_kit/registry.rb b/lib/fixture_kit/registry.rb index be45c99..bea55aa 100644 --- a/lib/fixture_kit/registry.rb +++ b/lib/fixture_kit/registry.rb @@ -8,6 +8,7 @@ def initialize @declarations = {} @fixtures = {} @resolving = [] + @cache_owners = {} end def add(name_or_definition, scope = nil) @@ -34,6 +35,23 @@ def fixtures @fixtures.values end + # Fixtures sharing a cache identifier share a cache file: the second one to + # generate finds the file already written, skips generation, and mounts the + # other's records. That is only safe when both declarations are the same, so + # anything else has to fail here -- otherwise it surfaces as a confusing + # NoMethodError on Repository, or not at all when both expose the same names. + # Identifiers are derived from the declaration site, so this is a backstop + # against a digest collision or a regression in that derivation. + def claim_cache_identifier(fixture) + owner = (@cache_owners[fixture.cache.identifier] ||= fixture) + return if owner.equal?(fixture) + return if owner.definition.fingerprint == fixture.definition.fingerprint + + raise FixtureKit::CacheIdentifierCollision, + "fixtures declared at #{owner.definition.location} and #{fixture.definition.location} " \ + "both resolve to the cache identifier '#{fixture.cache.identifier}'" + end + private def fetch_named_fixture(name) diff --git a/spec/dummy/script/rspec_queue_mode_sim.rb b/spec/dummy/script/rspec_queue_mode_sim.rb index 1e5a33d..042e6c6 100644 --- a/spec/dummy/script/rspec_queue_mode_sim.rb +++ b/spec/dummy/script/rspec_queue_mode_sim.rb @@ -10,7 +10,9 @@ module QueueModeSimulation BATCHES = [ ["spec/integration/queue_mode/batch_one.rb"], - ["spec/integration/queue_mode/batch_two_late.rb"] + ["spec/integration/queue_mode/batch_two_late.rb"], + ["spec/integration/queue_mode/batch_three_collision.rb"], + ["spec/integration/queue_mode/batch_four_collision.rb"] ] def run diff --git a/spec/dummy/spec/integration/fixture_kit_integration.rb b/spec/dummy/spec/integration/fixture_kit_integration.rb index c7abbb6..34d0437 100644 --- a/spec/dummy/spec/integration/fixture_kit_integration.rb +++ b/spec/dummy/spec/integration/fixture_kit_integration.rb @@ -139,11 +139,11 @@ module FixtureKitIntegrationTimeHelpers expect(fixture.anonymous_user.email).to eq("anonymous.fixture@example.com") normalized_scope = self.class.to_s.sub(/\ARSpec::ExampleGroups::/, "") - cache_file = File.join( - FixtureKit.runner.configuration.cache_path, - "_anonymous/#{ActiveSupport::Inflector.underscore(normalized_scope)}.json" - ) - expect(File.exist?(cache_file)).to be(true) + slug = ActiveSupport::Inflector.underscore(normalized_scope) + cache_files = Dir.glob( + File.join(FixtureKit.runner.configuration.cache_path, "_anonymous/**/*.json") + ).grep(%r{/#{Regexp.escape(slug)}\.[0-9a-f]{12}\.json\z}) + expect(cache_files.size).to eq(1) puts "FKIT_ASSERT:ANONYMOUS_FIXTURE" puts "FKIT_ASSERT:ANONYMOUS_CACHE_PATH" diff --git a/spec/dummy/spec/integration/queue_mode/batch_four_collision.rb b/spec/dummy/spec/integration/queue_mode/batch_four_collision.rb new file mode 100644 index 0000000..eb625dd --- /dev/null +++ b/spec/dummy/spec/integration/queue_mode/batch_four_collision.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +require "rails_helper" + +# Same description as batch_three_collision.rb. See the comment there. +RSpec.describe "Queue mode colliding description" do + fixture do + fourth = User.create!(name: "Queue Fourth", email: "queue.fourth@example.com") + + expose(fourth: fourth) + end + + it "mounts its own anonymous fixture rather than the earlier batch's" do + expect(fixture.fourth.name).to eq("Queue Fourth") + + puts "FKIT_ASSERT:QMODE_BATCH4_RAN" + end +end diff --git a/spec/dummy/spec/integration/queue_mode/batch_three_collision.rb b/spec/dummy/spec/integration/queue_mode/batch_three_collision.rb new file mode 100644 index 0000000..ff3c574 --- /dev/null +++ b/spec/dummy/spec/integration/queue_mode/batch_three_collision.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require "rails_helper" + +# Declares the same top-level description as batch_four_collision.rb, so both +# groups are named RSpec::ExampleGroups::QueueModeCollidingDescription: the +# constant is removed by RSpec::Core::World#reset between batches, so the +# duplicate-name counter never disambiguates them. +RSpec.describe "Queue mode colliding description" do + fixture do + third = User.create!(name: "Queue Third", email: "queue.third@example.com") + + expose(third: third) + end + + it "mounts its own anonymous fixture" do + expect(fixture.third.name).to eq("Queue Third") + + puts "FKIT_ASSERT:QMODE_BATCH3_RAN" + end +end diff --git a/spec/dummy/test/integration/fixture_kit_integration_test.rb b/spec/dummy/test/integration/fixture_kit_integration_test.rb index 598c13b..3f83add 100644 --- a/spec/dummy/test/integration/fixture_kit_integration_test.rb +++ b/spec/dummy/test/integration/fixture_kit_integration_test.rb @@ -100,11 +100,11 @@ class FixtureKitAnonymousFixtureIntegrationTest < ActiveSupport::TestCase assert_equal 1, @anonymous_user_count_in_setup assert_equal "anonymous.fixture@example.com", fixture.anonymous_user.email - cache_file = File.join( - FixtureKit.runner.configuration.cache_path, - "_anonymous/#{ActiveSupport::Inflector.underscore(self.class.name)}.json" - ) - assert File.exist?(cache_file) + slug = ActiveSupport::Inflector.underscore(self.class.name) + cache_files = Dir.glob( + File.join(FixtureKit.runner.configuration.cache_path, "_anonymous/**/*.json") + ).grep(%r{/#{Regexp.escape(slug)}\.[0-9a-f]{12}\.json\z}) + assert_equal 1, cache_files.size puts "FKIT_ASSERT:ANONYMOUS_FIXTURE" puts "FKIT_ASSERT:ANONYMOUS_CACHE_PATH" diff --git a/spec/integration/dummy_app_queue_mode_spec.rb b/spec/integration/dummy_app_queue_mode_spec.rb index e81a66f..78d9abc 100644 --- a/spec/integration/dummy_app_queue_mode_spec.rb +++ b/spec/integration/dummy_app_queue_mode_spec.rb @@ -34,6 +34,8 @@ def run_queue_mode_simulation "FKIT_ASSERT:QMODE_LATE_CACHE_ABSENT_AFTER_BATCH1", "FKIT_ASSERT:QMODE_BATCH2_RAN", "FKIT_ASSERT:QMODE_LATE_CACHE_GENERATED", + "FKIT_ASSERT:QMODE_BATCH3_RAN", + "FKIT_ASSERT:QMODE_BATCH4_RAN", "FKIT_ASSERT:QMODE_COMPLETE" ] diff --git a/spec/unit/definition_spec.rb b/spec/unit/definition_spec.rb index 58534d0..22a8b34 100644 --- a/spec/unit/definition_spec.rb +++ b/spec/unit/definition_spec.rb @@ -60,6 +60,37 @@ end end + describe "#location" do + it "returns the file and line where the definition block was defined" do + definition = described_class.new {} + + expect(definition.location).to eq("#{__FILE__}:#{__LINE__ - 2}") + end + end + + describe "#fingerprint" do + it "differs between declarations on different lines" do + first = described_class.new {} + second = described_class.new {} + + expect(first.fingerprint).not_to eq(second.fingerprint) + end + + it "differs between declarations extending different parents" do + definition_for = ->(extends) { described_class.new(extends: extends) {} } + + expect(definition_for.call("teams/basic").fingerprint).not_to eq( + definition_for.call("teams/admin").fingerprint + ) + end + + it "matches for two definitions built from the same declaration" do + definition_for = -> { described_class.new(extends: "teams/basic") {} } + + expect(definition_for.call.fingerprint).to eq(definition_for.call.fingerprint) + end + end + describe "#expose" do it "raises when the same name is exposed twice" do alice = User.create!(name: "Alice", email: "alice-duplicate@example.com") diff --git a/spec/unit/fixture_cache_spec.rb b/spec/unit/fixture_cache_spec.rb index 0926397..a1043fc 100644 --- a/spec/unit/fixture_cache_spec.rb +++ b/spec/unit/fixture_cache_spec.rb @@ -31,6 +31,23 @@ def identifier_for(identifier) end end + # Two example groups whose descriptions collapse to the same constant name + # stringify identically, so the scope alone cannot tell them apart. + def colliding_scope + Class.new.tap do |scope| + allow(scope).to receive(:to_s).and_return("RSpec::ExampleGroups::Foo::WithFixtureKit::Hello") + end + end + + def anonymous_fixture_double(scope: colliding_scope, definition: FixtureKit::Definition.new {}) + instance_double( + FixtureKit::Fixture, + identifier: scope, + definition: definition, + parent: nil + ) + end + before do allow(FixtureKit).to receive(:runner).and_return(runner) FileUtils.rm_rf(cache_path) @@ -58,18 +75,11 @@ def identifier_for(identifier) end it "normalizes class identifiers for anonymous fixtures" do - anonymous_scope = Class.new - allow(anonymous_scope).to receive(:to_s).and_return("RSpec::ExampleGroups::Foo::WithFixtureKit::Hello") - anonymous_definition = FixtureKit::Definition.new {} - anonymous_fixture = instance_double( - FixtureKit::Fixture, - identifier: anonymous_scope, - definition: anonymous_definition, - parent: nil - ) - anonymous_cache = described_class.new(anonymous_fixture) + anonymous_cache = described_class.new(anonymous_fixture_double) - expect(anonymous_cache.path).to eq(File.join(cache_path, "_anonymous/foo/with_fixture_kit/hello.json")) + expect(anonymous_cache.path).to match( + %r{\A#{Regexp.escape(File.join(cache_path, "_anonymous/foo/with_fixture_kit/hello"))}\.[0-9a-f]{12}\.json\z} + ) end end @@ -79,37 +89,55 @@ def identifier_for(identifier) end it "normalizes scope-class identifiers for anonymous fixtures" do - anonymous_scope = Class.new - allow(anonymous_scope).to receive(:to_s).and_return("RSpec::ExampleGroups::Foo::WithFixtureKit::Hello") - anonymous_definition = FixtureKit::Definition.new {} - anonymous_fixture = instance_double( - FixtureKit::Fixture, - identifier: anonymous_scope, - definition: anonymous_definition, - parent: nil - ) - anonymous_cache = described_class.new(anonymous_fixture) + anonymous_cache = described_class.new(anonymous_fixture_double) - expect(anonymous_cache.identifier).to eq("_anonymous/foo/with_fixture_kit/hello") + expect(anonymous_cache.identifier).to match( + %r{\A_anonymous/foo/with_fixture_kit/hello\.[0-9a-f]{12}\z} + ) end it "memoizes normalized anonymous identifiers" do - anonymous_scope = Class.new - allow(anonymous_scope).to receive(:to_s).and_return("RSpec::ExampleGroups::Foo::WithFixtureKit::Hello") - anonymous_definition = FixtureKit::Definition.new {} - anonymous_fixture = instance_double( - FixtureKit::Fixture, - identifier: anonymous_scope, - definition: anonymous_definition, - parent: nil - ) - anonymous_cache = described_class.new(anonymous_fixture) + anonymous_scope = colliding_scope + anonymous_cache = described_class.new(anonymous_fixture_double(scope: anonymous_scope)) anonymous_cache.identifier anonymous_cache.identifier expect(anonymous_scope).to have_received(:to_s).once end + + # Test frameworks derive the scope name from the group description, and that + # name is neither lossless nor -- under RSpec queue runners that reset the + # world between batches -- unique within a process. Two declarations that + # stringify the same way must still get their own cache entry, or the second + # one to generate silently mounts the first one's records. + it "distinguishes anonymous fixtures whose scopes stringify identically" do + first = described_class.new(anonymous_fixture_double(definition: FixtureKit::Definition.new {})) + second = described_class.new(anonymous_fixture_double(definition: FixtureKit::Definition.new {})) + + expect(first.identifier).not_to eq(second.identifier) + expect(first.identifier).to start_with("_anonymous/foo/with_fixture_kit/hello.") + expect(second.identifier).to start_with("_anonymous/foo/with_fixture_kit/hello.") + end + + # Same declaration site, different parent: sharing a cache entry here would + # mount the wrong parent's records. + it "distinguishes anonymous fixtures that extend different parents" do + definition_for = ->(extends) { FixtureKit::Definition.new(extends: extends) {} } + + first = described_class.new(anonymous_fixture_double(definition: definition_for.call("teams/basic"))) + second = described_class.new(anonymous_fixture_double(definition: definition_for.call("teams/admin"))) + + expect(first.identifier).not_to eq(second.identifier) + end + + it "reuses one identifier for the same declaration" do + definition = FixtureKit::Definition.new {} + + expect(described_class.new(anonymous_fixture_double(definition: definition)).identifier).to eq( + described_class.new(anonymous_fixture_double(definition: definition)).identifier + ) + end end describe "#exists?" do diff --git a/spec/unit/fixture_registry_spec.rb b/spec/unit/fixture_registry_spec.rb index 80edead..b59515a 100644 --- a/spec/unit/fixture_registry_spec.rb +++ b/spec/unit/fixture_registry_spec.rb @@ -13,6 +13,76 @@ allow(FixtureKit).to receive(:runner).and_return(runner) end + describe "#claim_cache_identifier" do + # Cache identifiers are derived from the declaration site, so two distinct + # declarations reaching this point at all means the derivation broke. The + # identifiers are stubbed here because that is the only way to get there. + def fixture_for(definition, identifier: "_anonymous/same_identifier") + instance_double( + FixtureKit::Fixture, + cache: instance_double(FixtureKit::Cache, identifier: identifier), + definition: definition + ) + end + + it "allows a fixture to claim its own identifier repeatedly" do + registry = described_class.new + fixture = fixture_for(FixtureKit::Definition.new {}) + + registry.claim_cache_identifier(fixture) + + expect { registry.claim_cache_identifier(fixture) }.not_to raise_error + end + + it "allows two fixtures built from the same declaration to share an identifier" do + registry = described_class.new + definition_for = -> { FixtureKit::Definition.new {} } + registry.claim_cache_identifier(fixture_for(definition_for.call)) + + expect do + registry.claim_cache_identifier(fixture_for(definition_for.call)) + end.not_to raise_error + end + + it "allows different declarations to hold different identifiers" do + registry = described_class.new + registry.claim_cache_identifier(fixture_for(FixtureKit::Definition.new {}, identifier: "_anonymous/first")) + + expect do + registry.claim_cache_identifier(fixture_for(FixtureKit::Definition.new {}, identifier: "_anonymous/second")) + end.not_to raise_error + end + + it "raises when two different declarations resolve to one identifier" do + registry = described_class.new + registry.claim_cache_identifier(fixture_for(FixtureKit::Definition.new {})) + + expect do + registry.claim_cache_identifier(fixture_for(FixtureKit::Definition.new {})) + end.to raise_error( + FixtureKit::CacheIdentifierCollision, + %r{both resolve to the cache identifier '_anonymous/same_identifier'} + ) + end + + it "names both declaration sites in the error" do + registry = described_class.new + first = FixtureKit::Definition.new {} + first_line = __LINE__ - 1 + second = FixtureKit::Definition.new {} + second_line = __LINE__ - 1 + + registry.claim_cache_identifier(fixture_for(first)) + + expect do + registry.claim_cache_identifier(fixture_for(second)) + end.to raise_error( + FixtureKit::CacheIdentifierCollision, + /#{Regexp.escape("#{__FILE__}:#{first_line}")} and #{Regexp.escape("#{__FILE__}:#{second_line}")}/ + ) + end + end + describe "#add" do it "loads and returns a fixture by name for a scope" do registry = described_class.new