From 0ab6c6e24e089727885d29974506aeb09809f82c Mon Sep 17 00:00:00 2001
From: Sam Saffron <sam.saffron@gmail.com>
Date: Fri, 10 May 2019 11:37:37 +1000
Subject: [PATCH] PERF: speed up spec suite, avoid featuring topics

Before: 6:05
After: 5:42

Featuring topics for `list/categories` is a very expensive operation that
happened each time we created a topic. This introduces a test only bypass
---
 app/models/topic.rb               |  9 +++++++--
 spec/models/category_list_spec.rb | 29 ++++++++++++++++++-----------
 spec/rails_helper.rb              |  3 +++
 3 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/app/models/topic.rb b/app/models/topic.rb
index 02fe6ddb9c1..8b38756ec59 100644
--- a/app/models/topic.rb
+++ b/app/models/topic.rb
@@ -671,6 +671,8 @@ class Topic < ActiveRecord::Base
     SQL
   end
 
+  cattr_accessor :update_featured_topics
+
   def changed_to_category(new_category)
     return true if new_category.blank? || Category.exists?(topic_id: id)
     return false if new_category.id == SiteSetting.uncategorized_category_id && !SiteSetting.allow_uncategorized_topics
@@ -701,8 +703,11 @@ class Topic < ActiveRecord::Base
       end
 
       Category.where(id: new_category.id).update_all("topic_count = topic_count + 1")
-      CategoryFeaturedTopic.feature_topics_for(old_category) unless @import_mode
-      CategoryFeaturedTopic.feature_topics_for(new_category) unless @import_mode || old_category.try(:id) == new_category.id
+
+      if Topic.update_featured_topics != false
+        CategoryFeaturedTopic.feature_topics_for(old_category) unless @import_mode
+        CategoryFeaturedTopic.feature_topics_for(new_category) unless @import_mode || old_category.try(:id) == new_category.id
+      end
     end
 
     true
diff --git a/spec/models/category_list_spec.rb b/spec/models/category_list_spec.rb
index a3b5bf3b950..93bee278a00 100644
--- a/spec/models/category_list_spec.rb
+++ b/spec/models/category_list_spec.rb
@@ -4,6 +4,10 @@ require 'rails_helper'
 require 'category_list'
 
 describe CategoryList do
+  before do
+    # we need automatic updating here cause we are testing this
+    Topic.update_featured_topics = true
+  end
 
   fab!(:user) { Fabricate(:user) }
   fab!(:admin) { Fabricate(:admin) }
@@ -66,7 +70,7 @@ describe CategoryList do
     fab!(:topic_category) { Fabricate(:category, num_featured_topics: 2) }
 
     context "with a topic in a category" do
-      fab!(:topic) { Fabricate(:topic, category: topic_category) }
+      let(:topic) { Fabricate(:topic, category: topic_category) }
       let(:category) { category_list.categories.find { |c| c.id == topic_category.id } }
 
       it "should return the category" do
@@ -77,19 +81,22 @@ describe CategoryList do
     end
 
     context "with pinned topics in a category" do
-      fab!(:topic1) { Fabricate(:topic, category: topic_category, bumped_at: 8.minutes.ago) }
-      fab!(:topic2) { Fabricate(:topic, category: topic_category, bumped_at: 5.minutes.ago) }
-      fab!(:topic3) { Fabricate(:topic, category: topic_category, bumped_at: 2.minutes.ago) }
-      fab!(:pinned) { Fabricate(:topic, category: topic_category, pinned_at: 10.minutes.ago, bumped_at: 10.minutes.ago) }
-      let(:category) { category_list.categories.find { |c| c.id == topic_category.id } }
+      let!(:topic1) { Fabricate(:topic, category: topic_category, bumped_at: 8.minutes.ago) }
+      let!(:topic2) { Fabricate(:topic, category: topic_category, bumped_at: 5.minutes.ago) }
+      let!(:topic3) { Fabricate(:topic, category: topic_category, bumped_at: 2.minutes.ago) }
+      let!(:pinned) { Fabricate(:topic, category: topic_category, pinned_at: 10.minutes.ago, bumped_at: 10.minutes.ago) }
 
-      it "returns pinned topic first" do
-        expect(category.displayable_topics.map(&:id)).to eq([pinned.id, topic3.id])
+      def displayable_topics
+        category_list = CategoryList.new(Guardian.new(user), include_topics: true)
+        category_list.categories.find { |c| c.id == topic_category.id }.displayable_topics.map(&:id)
       end
 
-      it "returns topics in bumped_at order if pinned was unpinned" do
-        PinnedCheck.stubs(:unpinned?).returns(true)
-        expect(category.displayable_topics.map(&:id)).to eq([topic3.id, topic2.id])
+      it "returns pinned topic first" do
+        expect(displayable_topics).to eq([pinned.id, topic3.id])
+
+        TopicUser.change(user.id, pinned.id, cleared_pinned_at: pinned.pinned_at + 10)
+
+        expect(displayable_topics).to eq([topic3.id, topic2.id])
       end
     end
 
diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb
index 47c267a0ace..3d5a92dd5b0 100644
--- a/spec/rails_helper.rb
+++ b/spec/rails_helper.rb
@@ -117,6 +117,9 @@ module TestSetup
       $test_cleanup_callbacks = nil
     end
 
+    # in test this is very expensive, we explicitly enable when needed
+    Topic.update_featured_topics = false
+
     # Running jobs are expensive and most of our tests are not concern with
     # code that runs inside jobs. run_later! means they are put on the redis
     # queue and never processed.