From 88650a125916638f298615d255aa2692fcaac1ee Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Thu, 9 May 2019 13:28:28 +1000 Subject: [PATCH] PERF: avoid checking for consecutive replies in test This check can issue up to 2 queries per post created, we have specific tests for it so we can avoid. This also rolls back #4da6ca4d --- config/environments/test.rb | 1 + spec/components/post_revisor_spec.rb | 2 +- spec/fabricators/post_fabricator.rb | 5 - spec/integration/watched_words_spec.rb | 2 +- spec/models/post_spec.rb | 128 ++++++++++++------------- 5 files changed, 67 insertions(+), 71 deletions(-) diff --git a/config/environments/test.rb b/config/environments/test.rb index 9186e924e00..e140ec56fb4 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -63,6 +63,7 @@ Discourse::Application.configure do s.set_regardless_of_locale(:crawl_images, false) s.set_regardless_of_locale(:download_remote_images_to_local, false) s.set_regardless_of_locale(:unique_posts_mins, 0) + s.set_regardless_of_locale(:max_consecutive_replies, 0) # disable plugins if ENV['LOAD_PLUGINS'] == '1' s.set_regardless_of_locale(:discourse_narrative_bot_enabled, false) diff --git a/spec/components/post_revisor_spec.rb b/spec/components/post_revisor_spec.rb index ebfd3434066..cdc23952eaf 100644 --- a/spec/components/post_revisor_spec.rb +++ b/spec/components/post_revisor_spec.rb @@ -102,7 +102,7 @@ describe PostRevisor do end context 'revise' do - let(:post) { Fabricate(:post_with_validation, post_args) } + let(:post) { Fabricate(:post, post_args) } let(:first_version_at) { post.last_version_at } subject { PostRevisor.new(post) } diff --git a/spec/fabricators/post_fabricator.rb b/spec/fabricators/post_fabricator.rb index dfa1b1c6b14..219a165528f 100644 --- a/spec/fabricators/post_fabricator.rb +++ b/spec/fabricators/post_fabricator.rb @@ -4,14 +4,9 @@ Fabricator(:post) do user topic { |attrs| Fabricate(:topic, user: attrs[:user]) } raw "Hello world" - skip_validation true post_type Post.types[:regular] end -Fabricator(:post_with_validation, from: :post) do - skip_validation false -end - Fabricator(:post_with_long_raw_content, from: :post) do raw 'This is a sample post with semi-long raw content. The raw content is also more than two hundred characters to satisfy any test conditions that require content longer diff --git a/spec/integration/watched_words_spec.rb b/spec/integration/watched_words_spec.rb index 126fff6c7a7..d8d127ef260 100644 --- a/spec/integration/watched_words_spec.rb +++ b/spec/integration/watched_words_spec.rb @@ -63,7 +63,7 @@ describe WatchedWord do end it "blocks on revisions" do - post = Fabricate(:post_with_validation, topic: Fabricate(:topic, user: tl2_user), user: tl2_user) + post = Fabricate(:post, topic: Fabricate(:topic, user: tl2_user), user: tl2_user) expect { PostRevisor.new(post).revise!(post.user, { raw: "Want some #{block_word.word} for cheap?" }, revised_at: post.updated_at + 10.seconds) expect(post.errors).to be_present diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index 261cc51383b..b6edeb6bf0a 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -57,7 +57,7 @@ describe Post do def post_with_body(body, user = nil) args = post_args.merge(raw: body) args[:user] = user if user.present? - Fabricate.build(:post_with_validation, args) + Fabricate.build(:post, args) end it { is_expected.to validate_presence_of :raw } @@ -79,7 +79,7 @@ describe Post do describe '#by_newest' do it 'returns posts ordered by created_at desc' do 2.times do |t| - Fabricate(:post_with_validation, created_at: t.seconds.from_now) + Fabricate(:post, created_at: t.seconds.from_now) end expect(Post.by_newest.first.created_at).to be > Post.by_newest.last.created_at end @@ -87,7 +87,7 @@ describe Post do describe '#with_user' do it 'gives you a user' do - Fabricate(:post_with_validation, user: Fabricate.build(:user)) + Fabricate(:post, user: Fabricate.build(:user)) expect(Post.with_user.first.user).to be_a User end end @@ -97,7 +97,7 @@ describe Post do describe "revisions and deleting/recovery" do context 'a post without links' do - let(:post) { Fabricate(:post_with_validation, post_args) } + let(:post) { Fabricate(:post, post_args) } before do post.trash! @@ -137,7 +137,7 @@ describe Post do context 'a post with notices' do let(:post) { - post = Fabricate(:post_with_validation, post_args) + post = Fabricate(:post, post_args) post.custom_fields["notice_type"] = Post.notices[:returning_user] post.custom_fields["notice_args"] = 1.day.ago post.save_custom_fields @@ -155,7 +155,7 @@ describe Post do end describe 'flagging helpers' do - fab!(:post) { Fabricate(:post_with_validation) } + fab!(:post) { Fabricate(:post) } fab!(:user) { Fabricate(:coding_horror) } fab!(:admin) { Fabricate(:admin) } @@ -198,7 +198,7 @@ describe Post do describe "maximum images" do fab!(:newuser) { Fabricate(:user, trust_level: TrustLevel[0]) } - let(:post_no_images) { Fabricate.build(:post_with_validation, post_args.merge(user: newuser)) } + let(:post_no_images) { Fabricate.build(:post, post_args.merge(user: newuser)) } let(:post_one_image) { post_with_body("![sherlock](http://bbc.co.uk/sherlock.jpg)", newuser) } let(:post_two_images) { post_with_body(" ", newuser) } let(:post_with_avatars) { post_with_body('smiley wink', newuser) } @@ -210,7 +210,7 @@ describe Post do let(:post_with_two_classy_images) { post_with_body(" ", newuser) } it "returns 0 images for an empty post" do - expect(Fabricate.build(:post_with_validation).image_count).to eq(0) + expect(Fabricate.build(:post).image_count).to eq(0) end it "finds images from markdown" do @@ -312,12 +312,12 @@ describe Post do describe "maximum attachments" do fab!(:newuser) { Fabricate(:user, trust_level: TrustLevel[0]) } - let(:post_no_attachments) { Fabricate.build(:post_with_validation, post_args.merge(user: newuser)) } + let(:post_no_attachments) { Fabricate.build(:post, post_args.merge(user: newuser)) } let(:post_one_attachment) { post_with_body('file.txt', newuser) } let(:post_two_attachments) { post_with_body('errors.log model.3ds', newuser) } it "returns 0 attachments for an empty post" do - expect(Fabricate.build(:post_with_validation).attachment_count).to eq(0) + expect(Fabricate.build(:post).attachment_count).to eq(0) end it "finds attachments from HTML" do @@ -431,7 +431,7 @@ describe Post do let(:post_with_mentions) { post_with_body("hello @#{newuser.username} how are you doing?", newuser) } it "returns 0 links for an empty post" do - expect(Fabricate.build(:post_with_validation).link_count).to eq(0) + expect(Fabricate.build(:post).link_count).to eq(0) end it "returns 0 links for a post with mentions" do @@ -503,44 +503,44 @@ describe Post do context 'raw_mentions' do it "returns an empty array with no matches" do - post = Fabricate.build(:post_with_validation, post_args.merge(raw: "Hello Jake and Finn!")) + post = Fabricate.build(:post, post_args.merge(raw: "Hello Jake and Finn!")) expect(post.raw_mentions).to eq([]) end it "returns lowercase unique versions of the mentions" do - post = Fabricate.build(:post_with_validation, post_args.merge(raw: "@Jake @Finn @Jake")) + post = Fabricate.build(:post, post_args.merge(raw: "@Jake @Finn @Jake")) expect(post.raw_mentions).to eq(['jake', 'finn']) end it "ignores pre" do # we need to force an inline - post = Fabricate.build(:post_with_validation, post_args.merge(raw: "p 
@Jake
@Finn")) + post = Fabricate.build(:post, post_args.merge(raw: "p 
@Jake
@Finn")) expect(post.raw_mentions).to eq(['finn']) end it "catches content between pre tags" do # per common mark we need to force an inline - post = Fabricate.build(:post_with_validation, post_args.merge(raw: "a 
hello
@Finn 
"))
+        post = Fabricate.build(:post, post_args.merge(raw: "a 
hello
 @Finn 
"))
         expect(post.raw_mentions).to eq(['finn'])
       end
 
       it "ignores code" do
-        post = Fabricate.build(:post_with_validation, post_args.merge(raw: "@Jake `@Finn`"))
+        post = Fabricate.build(:post, post_args.merge(raw: "@Jake `@Finn`"))
         expect(post.raw_mentions).to eq(['jake'])
       end
 
       it "ignores quotes" do
-        post = Fabricate.build(:post_with_validation, post_args.merge(raw: "[quote=\"Evil Trout\"]\n@Jake\n[/quote]\n@Finn"))
+        post = Fabricate.build(:post, post_args.merge(raw: "[quote=\"Evil Trout\"]\n@Jake\n[/quote]\n@Finn"))
         expect(post.raw_mentions).to eq(['finn'])
       end
 
       it "handles underscore in username" do
-        post = Fabricate.build(:post_with_validation, post_args.merge(raw: "@Jake @Finn @Jake_Old"))
+        post = Fabricate.build(:post, post_args.merge(raw: "@Jake @Finn @Jake_Old"))
         expect(post.raw_mentions).to eq(['jake', 'finn', 'jake_old'])
       end
 
       it "handles hyphen in groupname" do
-        post = Fabricate.build(:post_with_validation, post_args.merge(raw: "@org-board"))
+        post = Fabricate.build(:post, post_args.merge(raw: "@org-board"))
         expect(post.raw_mentions).to eq(['org-board'])
       end
 
@@ -590,11 +590,11 @@ describe Post do
 
   context 'validation' do
     it 'validates our default post' do
-      expect(Fabricate.build(:post_with_validation, post_args)).to be_valid
+      expect(Fabricate.build(:post, post_args)).to be_valid
     end
 
     it 'create blank posts as invalid' do
-      expect(Fabricate.build(:post_with_validation, raw: "")).not_to be_valid
+      expect(Fabricate.build(:post, raw: "")).not_to be_valid
     end
   end
 
@@ -627,7 +627,7 @@ describe Post do
 
   context 'revise' do
 
-    let(:post) { Fabricate(:post_with_validation, post_args) }
+    let(:post) { Fabricate(:post, post_args) }
     let(:first_version_at) { post.last_version_at }
 
     it 'has no revision' do
@@ -732,7 +732,7 @@ describe Post do
     let(:cooked) { "
\nSword reworks.gif1000x400 1000 KB\n
" }
 
     let(:post) do
-      Fabricate(:post_with_validation,
+      Fabricate(:post,
         raw: "",
         cooked: cooked
       )
@@ -746,7 +746,7 @@ describe Post do
 
   describe 'after save' do
 
-    let(:post) { Fabricate(:post_with_validation, post_args) }
+    let(:post) { Fabricate(:post, post_args) }
 
     it "has correct info set" do
       expect(post.user_deleted?).to eq(false)
@@ -762,8 +762,8 @@ describe Post do
 
     describe 'extract_quoted_post_numbers' do
 
-      let!(:post) { Fabricate(:post_with_validation, post_args) }
-      let(:reply) { Fabricate.build(:post_with_validation, post_args) }
+      let!(:post) { Fabricate(:post, post_args) }
+      let(:reply) { Fabricate.build(:post, post_args) }
 
       it "finds the quote when in the same topic" do
         reply.raw = "[quote=\"EvilTrout, post:#{post.post_number}, topic:#{post.topic_id}\"]hello[/quote]"
@@ -778,7 +778,7 @@ describe Post do
       end
 
       it "doesn't find the quote in the same post" do
-        reply = Fabricate.build(:post_with_validation, post_args.merge(post_number: 646))
+        reply = Fabricate.build(:post, post_args.merge(post_number: 646))
         reply.raw = "[quote=\"EvilTrout, post:#{reply.post_number}, topic:#{post.topic_id}\"]hello[/quote]"
         reply.extract_quoted_post_numbers
         expect(reply.quoted_post_numbers).to be_blank
@@ -790,7 +790,7 @@ describe Post do
       fab!(:topic) { Fabricate(:topic) }
       let(:other_user) { Fabricate(:coding_horror) }
       let(:reply_text) { "[quote=\"Evil Trout, post:1\"]\nhello\n[/quote]\nHmmm!" }
-      let!(:post) { PostCreator.new(topic.user, raw: Fabricate.build(:post_with_validation).raw, topic_id: topic.id).create }
+      let!(:post) { PostCreator.new(topic.user, raw: Fabricate.build(:post).raw, topic_id: topic.id).create }
       let!(:reply) { PostCreator.new(other_user, raw: reply_text, topic_id: topic.id, reply_to_post_number: post.post_number).create }
 
       it 'has a quote' do
@@ -841,10 +841,10 @@ describe Post do
   end
 
   context 'summary' do
-    let!(:p1) { Fabricate(:post_with_validation, post_args.merge(score: 4, percent_rank: 0.33)) }
-    let!(:p2) { Fabricate(:post_with_validation, post_args.merge(score: 10, percent_rank: 0.66)) }
-    let!(:p3) { Fabricate(:post_with_validation, post_args.merge(score: 5, percent_rank: 0.99)) }
-    fab!(:p4) { Fabricate(:post_with_validation, percent_rank: 0.99) }
+    let!(:p1) { Fabricate(:post, post_args.merge(score: 4, percent_rank: 0.33)) }
+    let!(:p2) { Fabricate(:post, post_args.merge(score: 10, percent_rank: 0.66)) }
+    let!(:p3) { Fabricate(:post, post_args.merge(score: 5, percent_rank: 0.99)) }
+    fab!(:p4) { Fabricate(:post, percent_rank: 0.99) }
 
     it "returns the OP and posts above the threshold in summary mode" do
       SiteSetting.summary_percent_filter = 66
@@ -856,9 +856,9 @@ describe Post do
   context 'sort_order' do
     context 'regular topic' do
 
-      let!(:p1) { Fabricate(:post_with_validation, post_args) }
-      let!(:p2) { Fabricate(:post_with_validation, post_args) }
-      let!(:p3) { Fabricate(:post_with_validation, post_args) }
+      let!(:p1) { Fabricate(:post, post_args) }
+      let!(:p2) { Fabricate(:post, post_args) }
+      let!(:p3) { Fabricate(:post, post_args) }
 
       it 'defaults to created order' do
         expect(Post.regular_order).to eq([p1, p2, p3])
@@ -868,10 +868,10 @@ describe Post do
 
   context "reply_history" do
 
-    let!(:p1) { Fabricate(:post_with_validation, post_args) }
-    let!(:p2) { Fabricate(:post_with_validation, post_args.merge(reply_to_post_number: p1.post_number)) }
-    let!(:p3) { Fabricate(:post_with_validation, post_args) }
-    let!(:p4) { Fabricate(:post_with_validation, post_args.merge(reply_to_post_number: p2.post_number)) }
+    let!(:p1) { Fabricate(:post, post_args) }
+    let!(:p2) { Fabricate(:post, post_args.merge(reply_to_post_number: p1.post_number)) }
+    let!(:p3) { Fabricate(:post, post_args) }
+    let!(:p4) { Fabricate(:post, post_args.merge(reply_to_post_number: p2.post_number)) }
 
     it "returns the posts in reply to this post" do
       expect(p4.reply_history).to eq([p1, p2])
@@ -885,12 +885,12 @@ describe Post do
   context "reply_ids" do
 
     fab!(:topic) { Fabricate(:topic) }
-    let!(:p1) { Fabricate(:post_with_validation, topic: topic, post_number: 1) }
-    let!(:p2) { Fabricate(:post_with_validation, topic: topic, post_number: 2, reply_to_post_number: 1) }
-    let!(:p3) { Fabricate(:post_with_validation, topic: topic, post_number: 3) }
-    let!(:p4) { Fabricate(:post_with_validation, topic: topic, post_number: 4, reply_to_post_number: 2) }
-    let!(:p5) { Fabricate(:post_with_validation, topic: topic, post_number: 5, reply_to_post_number: 4) }
-    let!(:p6) { Fabricate(:post_with_validation, topic: topic, post_number: 6) }
+    let!(:p1) { Fabricate(:post, topic: topic, post_number: 1) }
+    let!(:p2) { Fabricate(:post, topic: topic, post_number: 2, reply_to_post_number: 1) }
+    let!(:p3) { Fabricate(:post, topic: topic, post_number: 3) }
+    let!(:p4) { Fabricate(:post, topic: topic, post_number: 4, reply_to_post_number: 2) }
+    let!(:p5) { Fabricate(:post, topic: topic, post_number: 5, reply_to_post_number: 4) }
+    let!(:p6) { Fabricate(:post, topic: topic, post_number: 6) }
 
     before {
       PostReply.create!(post: p1, reply: p2)
@@ -927,8 +927,8 @@ describe Post do
 
     # integration test -> should move to centralized integration test
     it 'finds urls for posts presented' do
-      p1 = Fabricate(:post_with_validation)
-      p2 = Fabricate(:post_with_validation)
+      p1 = Fabricate(:post)
+      p2 = Fabricate(:post)
       expect(Post.urls([p1.id, p2.id])).to eq(p1.id => p1.url, p2.id => p2.url)
     end
   end
@@ -995,7 +995,7 @@ describe Post do
 
       describe 'when user can not mention a group' do
         it "should not create the mention" do
-          post = Fabricate(:post_with_validation, raw: "hello @#{group.name}")
+          post = Fabricate(:post, raw: "hello @#{group.name}")
           post.trigger_post_process
           post.reload
 
@@ -1027,7 +1027,7 @@ describe Post do
     let(:raw) { "hello from my site http://www.example.net http://#{GlobalSetting.hostname} http://#{RailsMultisite::ConnectionManagement.current_hostname}" }
 
     it "correctly detects host spam" do
-      post = Fabricate(:post_with_validation, raw: raw)
+      post = Fabricate(:post, raw: raw)
 
       expect(post.total_hosts_usage).to eq("www.example.net" => 1)
       post.acting_user.trust_level = 0
@@ -1045,7 +1045,7 @@ describe Post do
     it "doesn't punish staged users" do
       SiteSetting.newuser_spam_host_threshold = 1
       user = Fabricate(:user, staged: true, trust_level: 0)
-      post = Fabricate(:post_with_validation, raw: raw, user: user)
+      post = Fabricate(:post, raw: raw, user: user)
       expect(post.has_host_spam?).to eq(false)
     end
 
@@ -1055,7 +1055,7 @@ describe Post do
       user = Fabricate(:user, staged: true, trust_level: 0)
       user.created_at = 1.hour.ago
       user.unstage
-      post = Fabricate(:post_with_validation, raw: raw, user: user)
+      post = Fabricate(:post, raw: raw, user: user)
       expect(post.has_host_spam?).to eq(true)
     end
 
@@ -1065,20 +1065,20 @@ describe Post do
       user = Fabricate(:user, staged: true, trust_level: 0)
       user.created_at = 1.day.ago
       user.unstage
-      post = Fabricate(:post_with_validation, raw: raw, user: user)
+      post = Fabricate(:post, raw: raw, user: user)
       expect(post.has_host_spam?).to eq(false)
     end
 
     it "ignores private messages" do
       SiteSetting.newuser_spam_host_threshold = 1
       user = Fabricate(:user, trust_level: 0)
-      post = Fabricate(:post_with_validation, raw: raw, user: user, topic: Fabricate(:private_message_topic, user: user))
+      post = Fabricate(:post, raw: raw, user: user, topic: Fabricate(:private_message_topic, user: user))
       expect(post.has_host_spam?).to eq(false)
     end
   end
 
   it "has custom fields" do
-    post = Fabricate(:post_with_validation)
+    post = Fabricate(:post)
     expect(post.custom_fields["a"]).to eq(nil)
 
     post.custom_fields["Tommy"] = "Hanks"
@@ -1110,7 +1110,7 @@ describe Post do
   end
 
   describe "#set_owner" do
-    fab!(:post) { Fabricate(:post_with_validation) }
+    fab!(:post) { Fabricate(:post) }
     fab!(:coding_horror) { Fabricate(:coding_horror) }
 
     it "will change owner of a post correctly" do
@@ -1222,7 +1222,7 @@ describe Post do
   end
 
   it "automatically orders post revisions by number ascending" do
-    post = Fabricate(:post_with_validation)
+    post = Fabricate(:post)
     post.revisions.create!(user_id: 1, post_id: post.id, number: 2)
     post.revisions.create!(user_id: 1, post_id: post.id, number: 1)
     expect(post.revisions.pluck(:number)).to eq([1, 2])
@@ -1274,7 +1274,7 @@ describe Post do
       RAW
     end
 
-    let(:post) { Fabricate(:post_with_validation, raw: raw) }
+    let(:post) { Fabricate(:post, raw: raw) }
 
     it "finds all the uploads in the post" do
       post.custom_fields[Post::DOWNLOADED_IMAGES] = {
@@ -1341,13 +1341,13 @@ describe Post do
   context "have_uploads" do
     it "should find all posts with the upload" do
       ids = []
-      ids << Fabricate(:post_with_validation, cooked: "A post with upload ").id
-      ids << Fabricate(:post_with_validation, cooked: "A post with optimized image ").id
-      Fabricate(:post_with_validation)
-      ids << Fabricate(:post_with_validation, cooked: "A post with upload ").id
-      ids << Fabricate(:post_with_validation, cooked: "A post with upload link ").id
-      ids << Fabricate(:post_with_validation, cooked: "A post with optimized image ").id
-      Fabricate(:post_with_validation, cooked: "A post with external link ")
+      ids << Fabricate(:post, cooked: "A post with upload ").id
+      ids << Fabricate(:post, cooked: "A post with optimized image ").id
+      Fabricate(:post)
+      ids << Fabricate(:post, cooked: "A post with upload ").id
+      ids << Fabricate(:post, cooked: "A post with upload link ").id
+      ids << Fabricate(:post, cooked: "A post with optimized image ").id
+      Fabricate(:post, cooked: "A post with external link ")
       expect(Post.have_uploads.order(:id).pluck(:id)).to eq(ids)
     end
   end