Refine Playwright auth lifecycle

Author: lewispb

app/models/concerns/upright/playwright/form_authentication.rb

@@ -3,7 +3,6 @@ module Upright::Playwright::FormAuthentication
 
   included do
     class_attribute :authentication_service
-
     set_callback :page_ready, :after, :ensure_authenticated
   end
 

app/models/concerns/upright/playwright/lifecycle.rb

@@ -17,13 +17,7 @@ def user_agent
   private
     def with_browser(&block)
       ::Playwright.create(playwright_cli_executable_path: Upright.configuration.playwright_cli_path) do |playwright|
-        playwright.chromium.launch(headless: ENV.fetch("HEADLESS", "true") != "false", args: chromium_args, &block)
-      end
-    end
-
-    def chromium_args
-      [].tap do |args|
-        args << "--no-sandbox" if Rails.env.production? || ENV["PLAYWRIGHT_NO_SANDBOX"]
+        playwright.chromium.launch(headless: ENV.fetch("HEADLESS", "true") != "false", &block)
       end
     end
 
@@ -34,6 +28,7 @@ def with_context(browser, **options, &block)
       yield
     ensure
       run_callbacks :page_close do
+        # Rescue each step independently so one failed close does not block artifact capture
         page&.close rescue Rails.error.report($!)
         context&.close rescue Rails.error.report($!)
       end

app/models/concerns/upright/playwright/trace_recording.rb

@@ -2,7 +2,7 @@ module Upright::Playwright::TraceRecording
   extend ActiveSupport::Concern
 
   included do
-    attr_accessor :trace_path
+    attr_accessor :trace_artifacts
 
     set_callback :page_ready, :before, :start_trace
     set_callback :page_close, :before, :stop_trace
@@ -18,22 +18,27 @@ def start_trace
     end
 
     def stop_trace
-      self.trace_path = trace_dir.join("#{SecureRandom.hex}.zip").to_s
+      trace_path = trace_dir.join("#{SecureRandom.hex}.zip").to_s
       FileUtils.mkdir_p(trace_dir)
       context.tracing.stop(path: trace_path)
+
+      self.trace_artifacts ||= []
+      trace_artifacts << { label: current_recording_label, path: trace_path }
     rescue => error
       Rails.error.report(error)
-      self.trace_path = nil
     end
 
     def attach_trace(probe_result)
-      return unless trace_path && File.exist?(trace_path)
+      Array(trace_artifacts).each do |artifact|
+        next unless File.exist?(artifact.fetch(:path))
+
+        File.open(artifact.fetch(:path), "rb") do |file|
+          Upright::Artifact.new(name: recording_artifact_filename(artifact[:label], "zip"), content: file).attach_to(probe_result, timestamped: true)
+        end
 
-      File.open(trace_path, "rb") do |file|
-        Upright::Artifact.new(name: "#{probe_name}.zip", content: file).attach_to(probe_result, timestamped: true)
+        FileUtils.rm(artifact.fetch(:path))
       end
 
-      FileUtils.rm(trace_path)
-      self.trace_path = nil
+      self.trace_artifacts = []
     end
 end

app/models/concerns/upright/playwright/video_recording.rb

@@ -4,7 +4,7 @@ module Upright::Playwright::VideoRecording
   VIDEO_SIZE = { width: 1280, height: 720 }
 
   included do
-    attr_accessor :video_path, :video_object
+    attr_accessor :video_artifacts, :pending_video_recording
 
     set_callback :page_ready, :after, :capture_video_reference
     set_callback :page_close, :after, :finalize_video
@@ -33,28 +33,58 @@ def record_video?
     end
 
     def capture_video_reference
-      self.video_object = page.video if record_video?
+      return unless record_video?
+      return unless (video = page.video)
+
+      self.pending_video_recording = { label: current_recording_label, video: video }
     end
 
     def save_video
-      if video_object
-        self.video_path = video_dir.join("#{SecureRandom.hex}.webm").to_s
-        video_object.save_as(video_path)
-      end
+      return unless pending_video_recording
+
+      video_path = video_dir.join("#{SecureRandom.hex}.webm").to_s
+      pending_video_recording.fetch(:video).save_as(video_path)
+
+      self.video_artifacts ||= []
+      video_artifacts << pending_video_recording.merge(path: video_path)
+    ensure
+      self.pending_video_recording = nil
     end
 
     def attach_video(probe_result)
-      if video_path
-        File.open(video_path, "rb") do |file|
-          Upright::Artifact.new(name: "#{probe_name}.webm", content: file).attach_to(probe_result, timestamped: true)
-        end
+      Array(video_artifacts).each do |artifact|
+        next unless File.exist?(artifact.fetch(:path))
 
-        if logger.respond_to?(:struct) && probe_result.artifacts.any?
-          logger.struct probe_artifact_url: Rails.application.routes.url_helpers.rails_blob_url(probe_result.artifacts.first, expires_in: 24.hours)
+        File.open(artifact.fetch(:path), "rb") do |file|
+          Upright::Artifact.new(name: recording_artifact_filename(artifact[:label], "webm"), content: file).attach_to(probe_result, timestamped: true)
         end
 
-        FileUtils.rm(video_path)
-        self.video_path = nil
+        FileUtils.rm(artifact.fetch(:path))
       end
+
+      if logger.respond_to?(:struct)
+        video_artifact = probe_result.artifacts.find { |attached| attached.content_type == "video/webm" }
+        if video_artifact
+          logger.struct probe_artifact_url: Rails.application.routes.url_helpers.rails_blob_url(video_artifact, expires_in: 24.hours)
+        end
+      end
+
+      self.video_artifacts = []
+    end
+
+    def recording_artifact_filename(label, extension)
+      [ recording_artifact_basename(label), extension ].join(".")
+    end
+
+    def recording_artifact_basename(label)
+      [ artifact_recording_name, label ].compact.join(" ")
+    end
+
+    def artifact_recording_name
+      respond_to?(:probe_name) ? probe_name : service_name.to_s
+    end
+
+    def current_recording_label
+      nil
     end
 end

app/models/upright/playwright/authenticator/base.rb

@@ -14,7 +14,7 @@ def initialize
   def ensure_authenticated(context, page)
     @page = page
     load_cached_storage_state(context)
-    page.goto(signin_redirect_url, timeout: 10.seconds.in_ms)
+    page.goto(auth_check_url, timeout: 10.seconds.in_ms)
 
     unless session_valid_on?(page)
       authenticate_on(page)
@@ -60,6 +60,10 @@ def service_name
   end
 
   private
+    def auth_check_url
+      signin_redirect_url
+    end
+
     def authenticate
       raise NotImplementedError
     end

test/lib/helpers/mock_playwright_helper.rb

@@ -22,15 +22,26 @@ def closed? = @closed
 
   class MockTracing
     def start(**options) = nil
-    def stop(**options) = nil
+
+    def stop(path:)
+      FileUtils.mkdir_p(File.dirname(path))
+      File.write(path, "trace")
+    end
+  end
+
+  class MockVideo
+    def save_as(path)
+      FileUtils.mkdir_p(File.dirname(path))
+      File.write(path, "video")
+    end
   end
 
   class MockPage
     def goto(url, **options) = nil
     def url = "https://example.com/"
     def close = nil
     def on(event, callback) = nil
-    def video = nil
+    def video = MockVideo.new
     def wait_for_load_state(**options) = nil
   end
 end