From 2af03164cbf0c3e86b551d9be338780c204ef9fa Mon Sep 17 00:00:00 2001
From: Claire <claire.github-309c@sitedethib.com>
Date: Thu, 10 Feb 2022 19:42:45 +0100
Subject: Improve tests involving push_bulk (#17508)
sidekiq-bulk's push_bulk can either accept arguments directly or run them
through a block.
Setting expectations on the result of evaluating the blocks allows testing
more code (the block itself) and the test is moved closer to the *interface*
of the tested code than its precise implementation.
---
.../services/activitypub/fetch_replies_service_spec.rb | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
(limited to 'spec/services')
diff --git a/spec/services/activitypub/fetch_replies_service_spec.rb b/spec/services/activitypub/fetch_replies_service_spec.rb
index 65c453341..fe49b18c1 100644
--- a/spec/services/activitypub/fetch_replies_service_spec.rb
+++ b/spec/services/activitypub/fetch_replies_service_spec.rb
@@ -34,9 +34,8 @@ RSpec.describe ActivityPub::FetchRepliesService, type: :service do
context 'when the payload is a Collection with inlined replies' do
context 'when passing the collection itself' do
it 'spawns workers for up to 5 replies on the same server' do
- allow(FetchReplyWorker).to receive(:push_bulk)
+ expect(FetchReplyWorker).to receive(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
subject.call(status, payload)
- expect(FetchReplyWorker).to have_received(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
end
end
@@ -46,9 +45,8 @@ RSpec.describe ActivityPub::FetchRepliesService, type: :service do
end
it 'spawns workers for up to 5 replies on the same server' do
- allow(FetchReplyWorker).to receive(:push_bulk)
+ expect(FetchReplyWorker).to receive(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
subject.call(status, collection_uri)
- expect(FetchReplyWorker).to have_received(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
end
end
end
@@ -65,9 +63,8 @@ RSpec.describe ActivityPub::FetchRepliesService, type: :service do
context 'when passing the collection itself' do
it 'spawns workers for up to 5 replies on the same server' do
- allow(FetchReplyWorker).to receive(:push_bulk)
+ expect(FetchReplyWorker).to receive(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
subject.call(status, payload)
- expect(FetchReplyWorker).to have_received(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
end
end
@@ -77,9 +74,8 @@ RSpec.describe ActivityPub::FetchRepliesService, type: :service do
end
it 'spawns workers for up to 5 replies on the same server' do
- allow(FetchReplyWorker).to receive(:push_bulk)
+ expect(FetchReplyWorker).to receive(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
subject.call(status, collection_uri)
- expect(FetchReplyWorker).to have_received(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
end
end
end
@@ -100,9 +96,8 @@ RSpec.describe ActivityPub::FetchRepliesService, type: :service do
context 'when passing the collection itself' do
it 'spawns workers for up to 5 replies on the same server' do
- allow(FetchReplyWorker).to receive(:push_bulk)
+ expect(FetchReplyWorker).to receive(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
subject.call(status, payload)
- expect(FetchReplyWorker).to have_received(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
end
end
@@ -112,9 +107,8 @@ RSpec.describe ActivityPub::FetchRepliesService, type: :service do
end
it 'spawns workers for up to 5 replies on the same server' do
- allow(FetchReplyWorker).to receive(:push_bulk)
+ expect(FetchReplyWorker).to receive(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
subject.call(status, collection_uri)
- expect(FetchReplyWorker).to have_received(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5'])
end
end
end
--
cgit
From 472bc89611cb3ce0594cd7b42b730c3484899c33 Mon Sep 17 00:00:00 2001
From: Claire <claire.github-309c@sitedethib.com>
Date: Thu, 10 Feb 2022 22:00:10 +0100
Subject: Fix some flaky tests that randomly failed because of item ordering
(#17509)
---
.../followers_synchronizations_controller_spec.rb | 2 +-
.../api/v1/accounts/statuses_controller_spec.rb | 2 +-
.../models/account_statuses_cleanup_policy_spec.rb | 2 +-
spec/models/status_spec.rb | 32 +++++++++++-----------
spec/services/update_status_service_spec.rb | 2 +-
5 files changed, 20 insertions(+), 20 deletions(-)
(limited to 'spec/services')
diff --git a/spec/controllers/activitypub/followers_synchronizations_controller_spec.rb b/spec/controllers/activitypub/followers_synchronizations_controller_spec.rb
index 3a382ff27..e233bd560 100644
--- a/spec/controllers/activitypub/followers_synchronizations_controller_spec.rb
+++ b/spec/controllers/activitypub/followers_synchronizations_controller_spec.rb
@@ -47,7 +47,7 @@ RSpec.describe ActivityPub::FollowersSynchronizationsController, type: :controll
it 'returns orderedItems with followers from example.com' do
expect(body[:orderedItems]).to be_an Array
- expect(body[:orderedItems].sort).to eq [follower_4.uri, follower_1.uri, follower_2.uri]
+ expect(body[:orderedItems]).to match_array([follower_4.uri, follower_1.uri, follower_2.uri])
end
it 'returns private Cache-Control header' do
diff --git a/spec/controllers/api/v1/accounts/statuses_controller_spec.rb b/spec/controllers/api/v1/accounts/statuses_controller_spec.rb
index 348de08c2..b962b3398 100644
--- a/spec/controllers/api/v1/accounts/statuses_controller_spec.rb
+++ b/spec/controllers/api/v1/accounts/statuses_controller_spec.rb
@@ -79,7 +79,7 @@ describe Api::V1::Accounts::StatusesController do
it 'lists both the public and the private statuses' do
get :index, params: { account_id: account.id, pinned: true }
json = body_as_json
- expect(json.map { |item| item[:id].to_i }.sort).to eq [status.id, private_status.id].sort
+ expect(json.map { |item| item[:id].to_i }).to match_array([status.id, private_status.id])
end
end
end
diff --git a/spec/models/account_statuses_cleanup_policy_spec.rb b/spec/models/account_statuses_cleanup_policy_spec.rb
index 4732ad625..b01321a20 100644
--- a/spec/models/account_statuses_cleanup_policy_spec.rb
+++ b/spec/models/account_statuses_cleanup_policy_spec.rb
@@ -495,7 +495,7 @@ RSpec.describe AccountStatusesCleanupPolicy, type: :model do
end
it 'returns only normal statuses for deletion' do
- expect(subject.pluck(:id).sort).to eq [very_old_status.id, faved4.id, faved5.id, reblogged4.id, reblogged5.id].sort
+ expect(subject.pluck(:id)).to match_array([very_old_status.id, faved4.id, faved5.id, reblogged4.id, reblogged5.id])
end
end
diff --git a/spec/models/status_spec.rb b/spec/models/status_spec.rb
index 653575778..67af6d06d 100644
--- a/spec/models/status_spec.rb
+++ b/spec/models/status_spec.rb
@@ -279,17 +279,17 @@ RSpec.describe Status, type: :model do
context 'when given one tag' do
it 'returns the expected statuses' do
- expect(Status.tagged_with([tag1.id]).reorder(:id).pluck(:id).uniq).to eq [status1.id, status5.id]
- expect(Status.tagged_with([tag2.id]).reorder(:id).pluck(:id).uniq).to eq [status2.id, status5.id]
- expect(Status.tagged_with([tag3.id]).reorder(:id).pluck(:id).uniq).to eq [status3.id, status5.id]
+ expect(Status.tagged_with([tag1.id]).reorder(:id).pluck(:id).uniq).to match_array([status1.id, status5.id])
+ expect(Status.tagged_with([tag2.id]).reorder(:id).pluck(:id).uniq).to match_array([status2.id, status5.id])
+ expect(Status.tagged_with([tag3.id]).reorder(:id).pluck(:id).uniq).to match_array([status3.id, status5.id])
end
end
context 'when given multiple tags' do
it 'returns the expected statuses' do
- expect(Status.tagged_with([tag1.id, tag2.id]).reorder(:id).pluck(:id).uniq).to eq [status1.id, status2.id, status5.id]
- expect(Status.tagged_with([tag1.id, tag3.id]).reorder(:id).pluck(:id).uniq).to eq [status1.id, status3.id, status5.id]
- expect(Status.tagged_with([tag2.id, tag3.id]).reorder(:id).pluck(:id).uniq).to eq [status2.id, status3.id, status5.id]
+ expect(Status.tagged_with([tag1.id, tag2.id]).reorder(:id).pluck(:id).uniq).to match_array([status1.id, status2.id, status5.id])
+ expect(Status.tagged_with([tag1.id, tag3.id]).reorder(:id).pluck(:id).uniq).to match_array([status1.id, status3.id, status5.id])
+ expect(Status.tagged_with([tag2.id, tag3.id]).reorder(:id).pluck(:id).uniq).to match_array([status2.id, status3.id, status5.id])
end
end
end
@@ -306,15 +306,15 @@ RSpec.describe Status, type: :model do
context 'when given one tag' do
it 'returns the expected statuses' do
- expect(Status.tagged_with_all([tag1.id]).reorder(:id).pluck(:id).uniq).to eq [status1.id, status5.id]
- expect(Status.tagged_with_all([tag2.id]).reorder(:id).pluck(:id).uniq).to eq [status2.id, status5.id]
- expect(Status.tagged_with_all([tag3.id]).reorder(:id).pluck(:id).uniq).to eq [status3.id]
+ expect(Status.tagged_with_all([tag1.id]).reorder(:id).pluck(:id).uniq).to match_array([status1.id, status5.id])
+ expect(Status.tagged_with_all([tag2.id]).reorder(:id).pluck(:id).uniq).to match_array([status2.id, status5.id])
+ expect(Status.tagged_with_all([tag3.id]).reorder(:id).pluck(:id).uniq).to match_array([status3.id])
end
end
context 'when given multiple tags' do
it 'returns the expected statuses' do
- expect(Status.tagged_with_all([tag1.id, tag2.id]).reorder(:id).pluck(:id).uniq).to eq [status5.id]
+ expect(Status.tagged_with_all([tag1.id, tag2.id]).reorder(:id).pluck(:id).uniq).to match_array([status5.id])
expect(Status.tagged_with_all([tag1.id, tag3.id]).reorder(:id).pluck(:id).uniq).to eq []
expect(Status.tagged_with_all([tag2.id, tag3.id]).reorder(:id).pluck(:id).uniq).to eq []
end
@@ -333,17 +333,17 @@ RSpec.describe Status, type: :model do
context 'when given one tag' do
it 'returns the expected statuses' do
- expect(Status.tagged_with_none([tag1.id]).reorder(:id).pluck(:id).uniq).to eq [status2.id, status3.id, status4.id]
- expect(Status.tagged_with_none([tag2.id]).reorder(:id).pluck(:id).uniq).to eq [status1.id, status3.id, status4.id]
- expect(Status.tagged_with_none([tag3.id]).reorder(:id).pluck(:id).uniq).to eq [status1.id, status2.id, status4.id]
+ expect(Status.tagged_with_none([tag1.id]).reorder(:id).pluck(:id).uniq).to match_array([status2.id, status3.id, status4.id])
+ expect(Status.tagged_with_none([tag2.id]).reorder(:id).pluck(:id).uniq).to match_array([status1.id, status3.id, status4.id])
+ expect(Status.tagged_with_none([tag3.id]).reorder(:id).pluck(:id).uniq).to match_array([status1.id, status2.id, status4.id])
end
end
context 'when given multiple tags' do
it 'returns the expected statuses' do
- expect(Status.tagged_with_none([tag1.id, tag2.id]).reorder(:id).pluck(:id).uniq).to eq [status3.id, status4.id]
- expect(Status.tagged_with_none([tag1.id, tag3.id]).reorder(:id).pluck(:id).uniq).to eq [status2.id, status4.id]
- expect(Status.tagged_with_none([tag2.id, tag3.id]).reorder(:id).pluck(:id).uniq).to eq [status1.id, status4.id]
+ expect(Status.tagged_with_none([tag1.id, tag2.id]).reorder(:id).pluck(:id).uniq).to match_array([status3.id, status4.id])
+ expect(Status.tagged_with_none([tag1.id, tag3.id]).reorder(:id).pluck(:id).uniq).to match_array([status2.id, status4.id])
+ expect(Status.tagged_with_none([tag2.id, tag3.id]).reorder(:id).pluck(:id).uniq).to match_array([status1.id, status4.id])
end
end
end
diff --git a/spec/services/update_status_service_spec.rb b/spec/services/update_status_service_spec.rb
index fe1b60d24..4fd4837c6 100644
--- a/spec/services/update_status_service_spec.rb
+++ b/spec/services/update_status_service_spec.rb
@@ -114,7 +114,7 @@ RSpec.describe UpdateStatusService, type: :service do
end
it 'keeps old mentions as silent mentions' do
- expect(status.mentions.pluck(:account_id)).to eq [alice.id, bob.id]
+ expect(status.mentions.pluck(:account_id)).to match_array([alice.id, bob.id])
end
end
--
cgit
From d4e6774a0c88931d907f275821eb001e6dd2cb2d Mon Sep 17 00:00:00 2001
From: Claire <claire.github-309c@sitedethib.com>
Date: Fri, 11 Feb 2022 14:52:07 +0100
Subject: Fix Undo Announce sometimes inlining the originally Announced status
(#17516)
* Change tests to have more specific expectations on sent ActivityPub payloads
* Check that payload doesn't actually contain the contents of the boosted toot
* Fix Undo Announce sometimes inlining the originally Announced status
---
app/presenters/activitypub/activity_presenter.rb | 4 +-
.../activitypub/undo_announce_serializer.rb | 2 +-
spec/services/remove_status_service_spec.rb | 106 ++++++++++++++++-----
3 files changed, 87 insertions(+), 25 deletions(-)
(limited to 'spec/services')
diff --git a/app/presenters/activitypub/activity_presenter.rb b/app/presenters/activitypub/activity_presenter.rb
index 5d174767f..38e8527e8 100644
--- a/app/presenters/activitypub/activity_presenter.rb
+++ b/app/presenters/activitypub/activity_presenter.rb
@@ -4,7 +4,7 @@ class ActivityPub::ActivityPresenter < ActiveModelSerializers::Model
attributes :id, :type, :actor, :published, :to, :cc, :virtual_object
class << self
- def from_status(status)
+ def from_status(status, allow_inlining: true)
new.tap do |presenter|
presenter.id = ActivityPub::TagManager.instance.activity_uri_for(status)
presenter.type = status.reblog? ? 'Announce' : 'Create'
@@ -15,7 +15,7 @@ class ActivityPub::ActivityPresenter < ActiveModelSerializers::Model
presenter.virtual_object = begin
if status.reblog?
- if status.account == status.proper.account && status.proper.private_visibility? && status.local?
+ if allow_inlining && status.account == status.proper.account && status.proper.private_visibility? && status.local?
status.proper
else
ActivityPub::TagManager.instance.uri_for(status.proper)
diff --git a/app/serializers/activitypub/undo_announce_serializer.rb b/app/serializers/activitypub/undo_announce_serializer.rb
index a925efc18..a0f09e8ab 100644
--- a/app/serializers/activitypub/undo_announce_serializer.rb
+++ b/app/serializers/activitypub/undo_announce_serializer.rb
@@ -22,6 +22,6 @@ class ActivityPub::UndoAnnounceSerializer < ActivityPub::Serializer
end
def virtual_object
- ActivityPub::ActivityPresenter.from_status(object)
+ ActivityPub::ActivityPresenter.from_status(object, allow_inlining: false)
end
end
diff --git a/spec/services/remove_status_service_spec.rb b/spec/services/remove_status_service_spec.rb
index fb7c6b462..482068d58 100644
--- a/spec/services/remove_status_service_spec.rb
+++ b/spec/services/remove_status_service_spec.rb
@@ -15,35 +15,97 @@ RSpec.describe RemoveStatusService, type: :service do
jeff.follow!(alice)
hank.follow!(alice)
-
- @status = PostStatusService.new.call(alice, text: 'Hello @bob@example.com')
- FavouriteService.new.call(jeff, @status)
- Fabricate(:status, account: bill, reblog: @status, uri: 'hoge')
end
- it 'removes status from author\'s home feed' do
- subject.call(@status)
- expect(HomeFeed.new(alice).get(10)).to_not include(@status.id)
- end
+ context 'when removed status is not a reblog' do
+ before do
+ @status = PostStatusService.new.call(alice, text: 'Hello @bob@example.com ThisIsASecret')
+ FavouriteService.new.call(jeff, @status)
+ Fabricate(:status, account: bill, reblog: @status, uri: 'hoge')
+ end
- it 'removes status from local follower\'s home feed' do
- subject.call(@status)
- expect(HomeFeed.new(jeff).get(10)).to_not include(@status.id)
- end
+ it 'removes status from author\'s home feed' do
+ subject.call(@status)
+ expect(HomeFeed.new(alice).get(10)).to_not include(@status.id)
+ end
+
+ it 'removes status from local follower\'s home feed' do
+ subject.call(@status)
+ expect(HomeFeed.new(jeff).get(10)).to_not include(@status.id)
+ end
- it 'sends delete activity to followers' do
- subject.call(@status)
- expect(a_request(:post, 'http://example.com/inbox')).to have_been_made.twice
+ it 'sends Delete activity to followers' do
+ subject.call(@status)
+ expect(a_request(:post, 'http://example.com/inbox').with(
+ body: hash_including({
+ 'type' => 'Delete',
+ 'object' => {
+ 'type' => 'Tombstone',
+ 'id' => ActivityPub::TagManager.instance.uri_for(@status),
+ 'atomUri' => OStatus::TagManager.instance.uri_for(@status),
+ },
+ })
+ )).to have_been_made.once
+ end
+
+ it 'sends Delete activity to rebloggers' do
+ subject.call(@status)
+ expect(a_request(:post, 'http://example2.com/inbox').with(
+ body: hash_including({
+ 'type' => 'Delete',
+ 'object' => {
+ 'type' => 'Tombstone',
+ 'id' => ActivityPub::TagManager.instance.uri_for(@status),
+ 'atomUri' => OStatus::TagManager.instance.uri_for(@status),
+ },
+ })
+ )).to have_been_made.once
+ end
+
+ it 'remove status from notifications' do
+ expect { subject.call(@status) }.to change {
+ Notification.where(activity_type: 'Favourite', from_account: jeff, account: alice).count
+ }.from(1).to(0)
+ end
end
- it 'sends delete activity to rebloggers' do
- subject.call(@status)
- expect(a_request(:post, 'http://example2.com/inbox')).to have_been_made
+ context 'when removed status is a private self-reblog' do
+ before do
+ @original_status = Fabricate(:status, account: alice, text: 'Hello ThisIsASecret', visibility: :private)
+ @status = ReblogService.new.call(alice, @original_status)
+ end
+
+ it 'sends Undo activity to followers' do
+ subject.call(@status)
+ expect(a_request(:post, 'http://example.com/inbox').with(
+ body: hash_including({
+ 'type' => 'Undo',
+ 'object' => hash_including({
+ 'type' => 'Announce',
+ 'object' => ActivityPub::TagManager.instance.uri_for(@original_status),
+ }),
+ })
+ )).to have_been_made.once
+ end
end
- it 'remove status from notifications' do
- expect { subject.call(@status) }.to change {
- Notification.where(activity_type: 'Favourite', from_account: jeff, account: alice).count
- }.from(1).to(0)
+ context 'when removed status is public self-reblog' do
+ before do
+ @original_status = Fabricate(:status, account: alice, text: 'Hello ThisIsASecret', visibility: :public)
+ @status = ReblogService.new.call(alice, @original_status)
+ end
+
+ it 'sends Undo activity to followers' do
+ subject.call(@status)
+ expect(a_request(:post, 'http://example.com/inbox').with(
+ body: hash_including({
+ 'type' => 'Undo',
+ 'object' => hash_including({
+ 'type' => 'Announce',
+ 'object' => ActivityPub::TagManager.instance.uri_for(@original_status),
+ }),
+ })
+ )).to have_been_made.once
+ end
end
end
--
cgit