Add access to batches of tests fetched from the Queue API in RSpec, Minitest, Cucumber

[ArturT]

Story

Link to the internal story

Description

Applies to RSpec, Minitest, Cucumber in Queue Mode.

User can access info about batches of tests fetched from the Queue API. For example, you can access it within queue hooks like this:

KnapsackPro::Hooks::Queue.before_queue do |queue_id|
  print "Batches in before_queue: "
  puts KnapsackPro::Store::Client.batches.inspect
end

KnapsackPro::Hooks::Queue.before_subset_queue do |queue_id, subset_queue_id|
  print "Batches in before_subset_queue: "
  puts KnapsackPro::Store::Client.batches.map(&:test_file_paths).inspect
end

KnapsackPro::Hooks::Queue.after_subset_queue do |queue_id, subset_queue_id|
  print "Batches in after_subset_queue: "
  puts KnapsackPro::Store::Client.batches.map(&:test_file_paths).inspect
end

KnapsackPro::Hooks::Queue.after_queue do |queue_id|
  print "Batches in after_queue: "
  puts KnapsackPro::Store::Client.batches.map(&:test_file_paths).inspect
end

If you take a batch you can check if its tests has been executed and the batch status (at least one failing test case means the batch failed):

batch = KnapsackPro::Store::Client.batches.first
batch.test_file_paths
batch.executed? # true/false
batch.passed? # true/false or raises an exception (KnapsackPro::Store::TestBatch::BatchNotExecutedError) when the batch of tests is not executed yet

Checklist reminder

• You follow the architecture outlined below for RSpec in Queue Mode, which is a work in progress (feel free to propose changes):
  • Pure: lib/knapsack_pro/pure/queue/rspec_pure.rb contains pure functions that are unit tested.
  • Extension: lib/knapsack_pro/extensions/rspec_extension.rb encapsulates calls to RSpec internals and is integration and e2e tested.
  • Runner: lib/knapsack_pro/runners/queue/rspec_runner.rb invokes the pure code and the extension to produce side effects, which are integration and e2e tested.

[3v0k4]

Does this change create any problems if you run KSP locally and CTRL+c?

Left some comments, please let me know when you've applied what you think is valuable. I'll do a second round then.

[3v0k4]

Maybe let's add a link to the docs when it's documented?

[ArturT]

I'd like to add an exact example of how to use the new feature in the changelog. The changelog would remain accurate for a given version of the knapsack_pro gem if docs change in the future.

[3v0k4]

Client is confusing to me. What do you think about leaving the public interface on the KnapsackPro::Store level?

I much prefer KnapsackPro::Store.batches to KnapsackPro::Store::Client. The client part is leaking an implementation detail that a server/client exist internally.

[ArturT]

Should the Store still be a module or should I convert it to a class?

[3v0k4]

Some ideas in here:

  class Queue
      include Enumerable

      def initialize
        @batches = []
      end

      def each # https://ruby-doc.org/3.2.2/Enumerable.html#module-Enumerable-label-Usage

      def add_batch(batch)
        @batches << KnapsackPro::Store::TestBatch.new(batch)
      end

      def mark_batch_passed
         current_batch._passed
      end

      def mark_batch_failed
         current_batch._failed
      end

      private

      def current_batch
        @batches.last
      end
  end

[3v0k4]

This returns the object served by the server. But outside of this file, nobody knows there's a server/client.

What if we call it self.queue? See also the comments in lib/knapsack_pro/store/queue_batch_manager.rb for context.

[3v0k4]

This could raise if the pid is not found. Is there any chance it could happen that pid is not a running process? Maybe because of multiple calls to stop?

[ArturT]

Maybe because of multiple calls to stop?

Yes. I wanted our tests (spec/knapsack_pro/store/server_spec.rb) to be more robust if something fails in the middle or is terminated by the user. It's safe to always call the stop method (indirectly via the reset method) before you run a new test case even when process does not exist yet.

Is there any chance it could happen that pid is not a running process?

I hope not. I tested that. It is not possible to kill a child process (the store which is forked process) even with KILL -9. The parent process must be shutdown first. The parent would terminate the child process (the store).

Just in case I handle scenario when the PID does not exist and Errno::ESRCH is raised.

[3v0k4]

As promised, I'm leaving here some notes for reference since we are closing this PR.

I did a partial refactor of the Server that makes the tests pass:

# frozen_string_literal: true

module KnapsackPro
  module Store
    class Server
      extend Forwardable

      def self.start
        return if @server_pid

        assign_available_store_server_uri

        @server_pid = fork do
          queue = Queue.new
          Signal.trap("TERM") { queue.push(nil) }
          DRb.start_service(store_server_uri!, new)
          queue.pop # wait for TERM
          DRb.stop_service
          DRb.thread&.join
        end

        ::Kernel.at_exit { stop }
      end

      def self.client
        return @client unless @client.nil?

        DRb.start_service
        @client = DRbObject.new_with_uri(store_server_uri!)
        begin
          retries ||= 0
          @client.ping
          @client
        rescue DRb::DRbConnError
          wait_seconds = 0.1
          retries += wait_seconds
          sleep wait_seconds
          retry if retries <= 3 # seconds
          raise
        end
      end

      def_delegators :@queue_batch_manager, :add_batch, :last_batch_passed!, :last_batch_failed!, :batches

      def initialize
        @queue_batch_manager = KnapsackPro::Store::QueueBatchManager.new
      end

      def ping
        true
      end

      private

      def self.stop
        return if @server_pid.nil?
        Process.kill('TERM', @server_pid)
        Process.waitpid2(@server_pid)
      rescue Errno::ESRCH, Errno::ECHILD
        @server_pid = nil
      end

      def self.store_server_uri
        ENV['KNAPSACK_PRO_STORE_SERVER_URI']
      end

      def self.store_server_uri!
        store_server_uri || raise("KNAPSACK_PRO_STORE_SERVER_URI must be set to available DRb port.")
      end

      # must be set in the main/parent process to make the env var available to the child process
      def self.assign_available_store_server_uri
        return if store_server_uri

        DRb.start_service('druby://localhost:0')
        ENV['KNAPSACK_PRO_STORE_SERVER_URI'] = DRb.uri
        DRb.stop_service
      end
    end

    class TestableServer < Server # I would move this to the _spec file
      def self.reset
        stop
        ENV['KNAPSACK_PRO_STORE_SERVER_URI'] = nil
        @assigned_store_server_uri = nil
        @client = nil
      end
    end
  end
end

I wanted to propose renaming things, though I'm yet not convinced 100%:

• store.last_batch_passed! -> store.batch_passed!
• KnapsackPro::Store::Server.client -> KnapsackPro::Store::Internals.store
• KnapsackPro::Store::Server.start -> KnapsackPro::Store::Internals.start
• KnapsackPro::Store::QueueBatchManager -> KnapsackPro::Store::Queue::Internals
• KnapsackPro::Store::TestBatch -P> KnapsackPro::Store::Batch
• KnapsackPro::Store::Client -P> KnapsackPro::Store::Queue
• KnapsackPro::Store::Server.client.add_batch(test_file_paths) -> KnapsackPro::Store::Internals.store.add_batch(test_file_paths)
• KnapsackPro::Store::Client.batches.size -> KnapsackPro::Store::Queue.batches.size
• KnapsackPro::Store::Client.batches[0].test_file_paths -> KnapsackPro::Store::Queue.batches[0].test_file_paths
• KnapsackPro::Store::Client.batches[1].test_file_paths -> KnapsackPro::Store::Queue.batches[1].test_file_paths

We could have considered using sockets instead of TCP: DRb::DRbUNIXSocket.

Depending on the size of the payloads we may have looked into DRb::DRbUndumped.

Should have we looked more into GC? https://www.druby.org/sidruby/11-1-dealing-with-gc.html

dRuby Book