Skip to content

Commit

Permalink
Improve DiscoveryEngine::Sync tests
Browse files Browse the repository at this point in the history 
- Extract shared context and examples from tests
- Simplify `subject` logic in one place
  • Loading branch information
@csutter
csutter committed Aug 19, 2024
1 parent 73a7967 commit 001499b
Show file tree
Hide file tree
Showing 3 changed files with 128 additions and 185 deletions.
96 changes: 14 additions & 82 deletions spec/services/discovery_engine/sync/delete_spec.rb
View file
Original file line number Diff line number Diff line change
@@ -1,78 +1,44 @@
require_relative "shared_examples"

RSpec.describe DiscoveryEngine::Sync::Delete do
let(:client) { double("DocumentService::Client", delete_document: nil) }
let(:logger) { double("Logger", add: nil) }
subject(:sync) { described_class.new("some_content_id", payload_version: "1", client:) }

let(:lock) { instance_double(Coordination::DocumentLock, acquire: true, release: true) }
include_context "with sync context"

let(:version_cache) { instance_double(Coordination::DocumentVersionCache, sync_required?: sync_required, set_as_latest_synced_version: nil) }
let(:sync_required) { true }

before do
allow(Kernel).to receive(:sleep).and_return(nil)
allow(Rails).to receive(:logger).and_return(logger)
allow(Rails.configuration).to receive(:discovery_engine_datastore_branch).and_return("branch")
allow(GovukError).to receive(:notify)

allow(Coordination::DocumentLock).to receive(:new).with("some_content_id").and_return(lock)
allow(Coordination::DocumentVersionCache).to receive(:new)
.with("some_content_id", payload_version: "1").and_return(version_cache)
end

context "when the delete succeeds" do
before do
described_class.new("some_content_id", payload_version: "1", client:).call
sync.call
end

it "deletes the document" do
expect(client).to have_received(:delete_document)
.with(name: "branch/documents/some_content_id")
end

it "sets the new latest remote version" do
expect(version_cache).to have_received(:set_as_latest_synced_version)
end

it "logs the delete operation" do
expect(logger).to have_received(:add).with(
Logger::Severity::INFO,
"[DiscoveryEngine::Sync::Delete] Successful delete content_id:some_content_id payload_version:1",
)
end

it "acquires and releases the lock" do
expect(lock).to have_received(:acquire)
expect(lock).to have_received(:release)
end
it_behaves_like "a successful sync operation", "delete"
end

context "when the incoming document doesn't require syncing" do
let(:sync_required) { false }

before do
described_class.new("some_content_id", payload_version: "1", client:).call
sync.call
end

it "does not delete the document" do
expect(client).not_to have_received(:delete_document)
end

it "does not set the remote version" do
expect(version_cache).not_to have_received(:set_as_latest_synced_version)
end

it "logs that the document hasn't been deleted" do
expect(logger).to have_received(:add).with(
Logger::Severity::INFO,
"[DiscoveryEngine::Sync::Delete] Ignored as newer version already synced content_id:some_content_id payload_version:1",
)
end
it_behaves_like "a noop sync operation"
end

context "when locking the document fails" do
before do
allow(lock).to receive(:acquire).and_return(false)

described_class.new("some_content_id", payload_version: "1", client:).call
sync.call
end

it "deletes the document regardless" do
Expand All @@ -86,7 +52,7 @@
before do
allow(client).to receive(:delete_document).and_raise(err)

described_class.new("some_content_id", payload_version: "1", client:).call
sync.call
end

it "logs the failure" do
Expand All @@ -107,27 +73,10 @@
before do
allow(client).to receive(:delete_document).and_raise(err)

described_class.new("some_content_id", payload_version: "1", client:).call
sync.call
end

it "logs the failed attempts" do
expect(logger).to have_received(:add).with(
Logger::Severity::WARN,
"[DiscoveryEngine::Sync::Delete] Failed attempt 1 to delete document (Something went wrong), retrying content_id:some_content_id payload_version:1",
)
expect(logger).to have_received(:add).with(
Logger::Severity::WARN,
"[DiscoveryEngine::Sync::Delete] Failed attempt 2 to delete document (Something went wrong), retrying content_id:some_content_id payload_version:1",
)
expect(logger).to have_received(:add).with(
Logger::Severity::ERROR,
"[DiscoveryEngine::Sync::Delete] Failed on attempt 3 to delete document (Something went wrong), giving up content_id:some_content_id payload_version:1",
)
end

it "send the error to Sentry" do
expect(GovukError).to have_received(:notify).with(err)
end
it_behaves_like "a failed sync operation after the maximum attempts", "delete"
end

context "when failing transiently but succeeding within the maximum attempts" do
Expand All @@ -140,30 +89,13 @@
->(_) { double(name: "document-name") },
)

described_class.new("some_content_id", payload_version: "1", client:).call
sync.call
end

it "tries three times" do
expect(client).to have_received(:delete_document).exactly(3).times
end

it "logs the failed and successful attempts" do
expect(logger).to have_received(:add).with(
Logger::Severity::WARN,
"[DiscoveryEngine::Sync::Delete] Failed attempt 1 to delete document (Something went wrong), retrying content_id:some_content_id payload_version:1",
).ordered
expect(logger).to have_received(:add).with(
Logger::Severity::WARN,
"[DiscoveryEngine::Sync::Delete] Failed attempt 2 to delete document (Something went wrong), retrying content_id:some_content_id payload_version:1",
).ordered
expect(logger).to have_received(:add).with(
Logger::Severity::INFO,
"[DiscoveryEngine::Sync::Delete] Successful delete content_id:some_content_id payload_version:1",
).ordered
end

it "does not send an error to Sentry" do
expect(GovukError).not_to have_received(:notify)
end
it_behaves_like "a sync operation that eventually succeeds", "delete"
end
end
126 changes: 23 additions & 103 deletions spec/services/discovery_engine/sync/put_spec.rb
View file
Original file line number Diff line number Diff line change
@@ -1,36 +1,25 @@
RSpec.describe DiscoveryEngine::Sync::Put do
let(:client) { double("DocumentService::Client", update_document: nil) }
let(:logger) { double("Logger", add: nil) }

let(:lock) { instance_double(Coordination::DocumentLock, acquire: true, release: true) }
let(:version_cache) { instance_double(Coordination::DocumentVersionCache, sync_required?: sync_required, set_as_latest_synced_version: nil) }
let(:sync_required) { true }
require_relative "shared_examples"

before do
allow(Kernel).to receive(:sleep).and_return(nil)
allow(Rails).to receive(:logger).and_return(logger)
allow(Rails.configuration).to receive(:discovery_engine_datastore_branch).and_return("branch")
allow(GovukError).to receive(:notify)
RSpec.describe DiscoveryEngine::Sync::Put do
subject(:sync) do
described_class.new(
"some_content_id",
{ foo: "bar" },
content: "some content",
payload_version: "1",
client:,
)
end

allow(Coordination::DocumentLock).to receive(:new).with("some_content_id").and_return(lock)
include_context "with sync context"

allow(Coordination::DocumentVersionCache).to receive(:new)
.with("some_content_id", payload_version: "1").and_return(version_cache)
end
let(:sync_required) { true }

context "when updating the document succeeds" do
before do
allow(client).to receive(:update_document).and_return(
double(name: "document-name"),
)
allow(client).to receive(:update_document).and_return(double(name: "document-name"))

described_class.new(
"some_content_id",
{ foo: "bar" },
content: "some content",
payload_version: "1",
client:,
).call
sync.call
end

it "updates the document" do
Expand All @@ -48,63 +37,28 @@
)
end

it "acquires and releases the lock" do
expect(lock).to have_received(:acquire)
expect(lock).to have_received(:release)
end

it "sets the new latest remote version" do
expect(version_cache).to have_received(:set_as_latest_synced_version)
end

it "logs the put operation" do
expect(logger).to have_received(:add).with(
Logger::Severity::INFO,
"[DiscoveryEngine::Sync::Put] Successful put content_id:some_content_id payload_version:1",
)
end
it_behaves_like "a successful sync operation", "put"
end

context "when the incoming document doesn't need syncing" do
let(:sync_required) { false }

before do
described_class.new(
"some_content_id",
{ foo: "bar" },
content: "some content",
payload_version: "1",
client:,
).call
sync.call
end

it "does not update the document" do
expect(client).not_to have_received(:update_document)
end

it "does not set the remote version" do
expect(version_cache).not_to have_received(:set_as_latest_synced_version)
end

it "logs that the document hasn't been updated" do
expect(logger).to have_received(:add).with(
Logger::Severity::INFO,
"[DiscoveryEngine::Sync::Put] Ignored as newer version already synced content_id:some_content_id payload_version:1",
)
end
it_behaves_like "a noop sync operation"
end

context "when locking the document fails" do
before do
allow(lock).to receive(:acquire).and_return(false)

described_class.new(
"some_content_id",
{ foo: "bar" },
content: "some content",
payload_version: "1",
client:,
).call
sync.call
end

it "updates the document regardless" do
Expand All @@ -118,27 +72,10 @@
before do
allow(client).to receive(:update_document).and_raise(err)

described_class.new("some_content_id", {}, payload_version: "1", client:).call
sync.call
end

it "logs the failed attempts" do
expect(logger).to have_received(:add).with(
Logger::Severity::WARN,
"[DiscoveryEngine::Sync::Put] Failed attempt 1 to put document (Something went wrong), retrying content_id:some_content_id payload_version:1",
)
expect(logger).to have_received(:add).with(
Logger::Severity::WARN,
"[DiscoveryEngine::Sync::Put] Failed attempt 2 to put document (Something went wrong), retrying content_id:some_content_id payload_version:1",
)
expect(logger).to have_received(:add).with(
Logger::Severity::ERROR,
"[DiscoveryEngine::Sync::Put] Failed on attempt 3 to put document (Something went wrong), giving up content_id:some_content_id payload_version:1",
)
end

it "sends the error to Sentry" do
expect(GovukError).to have_received(:notify).with(err)
end
it_behaves_like "a failed sync operation after the maximum attempts", "put"
end

context "when failing transiently but succeeding within the maximum attempts" do
Expand All @@ -151,30 +88,13 @@
->(_) { double(name: "document-name") },
)

described_class.new("some_content_id", {}, payload_version: "1", client:).call
sync.call
end

it "tries three times" do
expect(client).to have_received(:update_document).exactly(3).times
end

it "logs the failed and successful attempts" do
expect(logger).to have_received(:add).with(
Logger::Severity::WARN,
"[DiscoveryEngine::Sync::Put] Failed attempt 1 to put document (Something went wrong), retrying content_id:some_content_id payload_version:1",
).ordered
expect(logger).to have_received(:add).with(
Logger::Severity::WARN,
"[DiscoveryEngine::Sync::Put] Failed attempt 2 to put document (Something went wrong), retrying content_id:some_content_id payload_version:1",
).ordered
expect(logger).to have_received(:add).with(
Logger::Severity::INFO,
"[DiscoveryEngine::Sync::Put] Successful put content_id:some_content_id payload_version:1",
).ordered
end

it "does not send an error to Sentry" do
expect(GovukError).not_to have_received(:notify)
end
it_behaves_like "a sync operation that eventually succeeds", "put"
end
end
Loading

0 comments on commit 001499b

Please sign in to comment.