[openstreetmap/openstreetmap-website] Optional Trace import using ActiveJob (#2120)

[Tom Hughes]

tomhughes commented on this pull request.



> @@ -0,0 +1,10 @@
+class TraceDestroyerJob < ApplicationJob
+  queue_as :default
+
+  def perform(trace)
+    trace.destroy
+  rescue StandardError => ex
+    logger.info ex.to_s
+    ex.backtrace.each { |l| logger.info l }

Why are we catching and logging exceptions? By doing that we make it look like the job has succeeded and it will be removed from the queue - if we didn't do that then it would stay queued...

> +
+class TraceImporterJobTest < ActiveJob::TestCase
+  def test_success_notification
+    # Check that the user gets a success notification when the trace has valid points
+    trace = create(:trace)
+
+    gpx = Minitest::Mock.new
+    def gpx.actual_points
+      5
+    end
+
+    trace.stub(:import, gpx) do
+      perform_enqueued_jobs do
+        TraceImporterJob.perform_now(trace)
+      end
+    end

I'm struggling to understand this, or to find any documentation of the `stub` method that would help me. I assume it's a FactoryBot thing but although I find occasional references to it on google the actual documentation never seems to mention it.

I assume it stubs the `import` method but I don't see when this test would ever call that method? or why calling import which would normally be called from the job should actually run the job?

I'm probably just being dense but I'd like to understand what's going on ;-)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/2120#pullrequestreview-196195708
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20190124/cbd4ffd4/attachment.html>