DEV: Fix failing chat spec and add unexpected failure indicator (#20299)

This commit fixes the UpdateUserLastRead spec which was checking
for a message ID that did not exist -- this could fail at times
since message ID 2 could exist. Better to create + destroy a message
since then it's guaranteed we have a unique ID.

This also attempts to clarify a step that we expect to fail which
succeeds instead by adding another emoji next to the success tick and
an explanation text.

Also removes some uses of unless in Services::Base, we generally prefer
to use alternatives, since unless can be hard to parse in a lot of
cases.

Co-authored-by: Loïc Guitaut <loic@discourse.org>

plugins/chat/app/services/base.rb

@@ -197,7 +197,7 @@ module Chat
         def call(instance, context)
           method = instance.method(method_name)
           args = {}
-          args = context.to_h unless method.arity.zero?
+          args = context.to_h if !method.arity.zero?
           context[result_key] = Context.build
           instance.instance_exec(**args, &method)
         end
@@ -217,7 +217,7 @@ module Chat
       class ModelStep < Step
         def call(instance, context)
           context[name] = super
-          raise ArgumentError, "Model not found" unless context[name]
+          raise ArgumentError, "Model not found" if !context[name]
         rescue ArgumentError => exception
           context[result_key].fail(exception: exception)
           context.fail!
@@ -227,7 +227,7 @@ module Chat
       # @!visibility private
       class PolicyStep < Step
         def call(instance, context)
-          unless super
+          if !super
             context[result_key].fail
             context.fail!
           end
@@ -250,7 +250,7 @@ module Chat
           contract = class_name.new(default_values.merge(context.to_h.slice(*attributes)))
           context[contract_name] = contract
           context[result_key] = Context.build
-          unless contract.valid?
+          if contract.invalid?
             context[result_key].fail(errors: contract.errors)
             context.fail!
           end
@@ -384,7 +384,7 @@ module Chat
       #   private
       #
       #   def save_channel(channel:, **)
-      #     fail!("something went wrong") unless channel.save
+      #     fail!("something went wrong") if !channel.save
       #   end
 
       # @!scope class

plugins/chat/lib/steps_inspector.rb

@@ -30,9 +30,7 @@ module Chat
       end
 
       def emoji
-        return "❌" if failure?
+        "#{result_emoji}#{unexpected_result_emoji}"
-        return "✅" if success?
-        ""
       end
 
       def steps
@@ -48,6 +46,21 @@ module Chat
       def step_result
         result["result.#{type}.#{name}"]
       end
+
+      def result_emoji
+        return "❌" if failure?
+        return "✅" if success?
+        ""
+      end
+
+      def unexpected_result_emoji
+        " ⚠️#{unexpected_result_text}" if step_result.try(:[], "spec.unexpected_result")
+      end
+
+      def unexpected_result_text
+        return "  <= expected to return true but got false instead" if failure?
+        "  <= expected to return false but got true instead"
+      end
     end
 
     # @!visibility private

plugins/chat/spec/lib/steps_inspector_spec.rb

@@ -131,6 +131,47 @@ RSpec.describe Chat::StepsInspector do
         OUTPUT
       end
     end
+
+    context "when running in specs" do
+      context "when a successful step is flagged as being an unexpected result" do
+        before { result["result.policy.policy"]["spec.unexpected_result"] = true }
+
+        it "adapts its output accordingly" do
+          expect(output).to eq <<~OUTPUT.chomp
+          [1/7] [model] 'model' ✅
+          [2/7] [policy] 'policy' ✅ ⚠️  <= expected to return false but got true instead
+          [3/7] [contract] 'default' ✅
+          [4/7] [transaction]
+          [5/7]   [step] 'in_transaction_step_1' ✅
+          [6/7]   [step] 'in_transaction_step_2' ✅
+          [7/7] [step] 'final_step' ✅
+          OUTPUT
+        end
+      end
+
+      context "when a failing step is flagged as being an unexpected result" do
+        before do
+          class DummyService
+            def policy
+              false
+            end
+          end
+          result["result.policy.policy"]["spec.unexpected_result"] = true
+        end
+
+        it "adapts its output accordingly" do
+          expect(output).to eq <<~OUTPUT.chomp
+          [1/7] [model] 'model' ✅
+          [2/7] [policy] 'policy' ❌ ⚠️  <= expected to return true but got false instead
+          [3/7] [contract] 'default' 
+          [4/7] [transaction]
+          [5/7]   [step] 'in_transaction_step_1' 
+          [6/7]   [step] 'in_transaction_step_2' 
+          [7/7] [step] 'final_step' 
+          OUTPUT
+        end
+      end
+    end
   end
 
   describe "#error" do

plugins/chat/spec/services/update_user_last_read_spec.rb

@@ -65,7 +65,9 @@ RSpec.describe Chat::Service::UpdateUserLastRead do
 
       context "when message doesn’t exist" do
         before do
-          params[:message_id] = 2
+          message = Fabricate(:chat_message)
+          params[:message_id] = message.id
+          message.trash!
           membership.update!(last_read_message_id: 1)
         end
 

plugins/chat/spec/support/chat_service_matcher.rb

@@ -15,6 +15,7 @@ module Chat
       end
 
       def failure_message
+        set_unexpected_result
         message =
           if !step_exists?
             "Expected #{type} '#{name}' (key: '#{step}') was not found in the result object."
@@ -27,6 +28,7 @@ module Chat
       end
 
       def failure_message_when_negated
+        set_unexpected_result
         message = "Expected #{type} '#{name}' (key: '#{step}') to succeed but it failed."
         error_message_with_inspection(message)
       end
@@ -61,6 +63,11 @@ module Chat
         inspector = StepsInspector.new(result)
         "#{message}\n\n#{inspector.inspect}\n\n#{inspector.error}"
       end
+
+      def set_unexpected_result
+        return unless result[step]
+        result[step]["spec.unexpected_result"] = true
+      end
     end
 
     class FailContract < FailStep