Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 51 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
a2959e8
Add KnapsackPro::Store::Server
ArturT May 7, 2024
b064356
wip
ArturT May 7, 2024
e33c5d2
wip
ArturT May 7, 2024
a3a9a84
wip
ArturT May 8, 2024
ba2b4f5
Update server.rb
ArturT May 8, 2024
4076cbf
wip
ArturT May 8, 2024
620de30
Update server.rb
ArturT May 8, 2024
2e5efb0
at_exit to stop drb server
ArturT May 8, 2024
a86bb07
wip
ArturT May 8, 2024
fb9bc6d
wip
ArturT May 8, 2024
4c4b0c4
wip
ArturT May 8, 2024
a9819b8
wip - log test files batches in queue mode for rspec, cucumber, minitest
ArturT May 8, 2024
75679a7
[wip] this must be refactor to save state of object via drb
ArturT May 9, 2024
93acf24
queue test batches
ArturT May 10, 2024
61690b5
refactor TestsBatch
ArturT May 10, 2024
799ae18
rename TestsBatch to TestBatch
ArturT May 10, 2024
a613b92
wip - handle delayed DRb start in a fork
ArturT May 10, 2024
f2b51d1
wip - flaky tests
ArturT May 10, 2024
0cf2ce3
refactor assign_store_server_uri
ArturT May 10, 2024
4ef38a4
Update server.rb
ArturT May 10, 2024
756ac6b
wip
ArturT May 10, 2024
0b06ea0
Update server.rb
ArturT May 11, 2024
c199f4e
try KILL instead of QUIT
ArturT May 11, 2024
03e218e
Update server.rb
ArturT May 11, 2024
b8d86d0
use TERM signal for graceful termination
ArturT May 11, 2024
2ef74eb
wip
ArturT May 11, 2024
550a7f1
wip
ArturT May 11, 2024
94fa4ef
Update server_spec.rb
ArturT May 11, 2024
ef860f1
fix flaky tests
ArturT May 13, 2024
e9d83ef
Update server.rb
ArturT May 13, 2024
c67c394
Update server_spec.rb
ArturT May 13, 2024
c305c53
refactor
ArturT May 13, 2024
9b430aa
refactor
ArturT May 13, 2024
1e12f60
Update server.rb
ArturT May 13, 2024
04eced6
add spec for queue base runner
ArturT May 13, 2024
c0d9dde
fix integration tests for rspec in queue mode
ArturT May 13, 2024
f9d58c7
collect info about passed/failed batches in RSpec
ArturT May 14, 2024
b8d19a3
update integration tests to verify store collects info about batches …
ArturT May 14, 2024
00b37f0
feat: collect info about batches of tests in Cucumber (queue mode)
ArturT May 14, 2024
4cd527a
feat: collect info about batches of tests in Minitest in Queue Mode
ArturT May 14, 2024
0bf51d9
Just empty commit
ArturT May 14, 2024
03c15ea
Update CHANGELOG.md
ArturT May 14, 2024
a7dc8ba
Update lib/knapsack_pro/store/test_batch.rb
ArturT May 15, 2024
bb371e8
rename exception to BatchNotExecutedError
ArturT May 15, 2024
b977b3b
Update lib/knapsack_pro/store/server.rb
ArturT May 15, 2024
b819328
refactor: extract TestableServer
ArturT May 15, 2024
d200ad9
remove redundant @assigned_store_server_uri
ArturT May 15, 2024
183f4ef
refactor: use Queue#pop to wait for the forked process until it's ter…
ArturT May 15, 2024
f027d8a
Revert "remove redundant @assigned_store_server_uri"
ArturT May 15, 2024
e474b49
refactor: do not add empty batch to the store
ArturT May 15, 2024
24cd850
fix: use fully qualified name Thread::Queue to avoid clashes with oth…
ArturT May 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
# Changelog

### 7.3.0

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


https://github.com/KnapsackPro/knapsack_pro-ruby/pull/252

https://github.com/KnapsackPro/knapsack_pro-ruby/compare/v7.2.0...v7.3.0

### 7.2.0

* Always use the original `Net::HTTP` client, even when WebMock replaces it with its own
Expand Down
6 changes: 6 additions & 0 deletions lib/knapsack_pro.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
require 'digest'
require 'securerandom'
require 'timeout'
require 'drb/drb'
require 'forwardable'
require_relative 'knapsack_pro/urls'
require_relative 'knapsack_pro/version'
require_relative 'knapsack_pro/hooks/queue'
Expand Down Expand Up @@ -77,6 +79,10 @@
require_relative 'knapsack_pro/runners/queue/rspec_runner'
require_relative 'knapsack_pro/runners/queue/cucumber_runner'
require_relative 'knapsack_pro/runners/queue/minitest_runner'
require_relative 'knapsack_pro/store/server'
require_relative 'knapsack_pro/store/client'
require_relative 'knapsack_pro/store/queue_batch_manager'
require_relative 'knapsack_pro/store/test_batch'
require_relative 'knapsack_pro/test_case_detectors/rspec_test_example_detector'
require_relative 'knapsack_pro/crypto/encryptor'
require_relative 'knapsack_pro/crypto/branch_encryptor'
Expand Down
9 changes: 7 additions & 2 deletions lib/knapsack_pro/extensions/rspec_extension.rb
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,20 @@ def knapsack__run_specs(queue_runner)

configuration.reporter.report(_expected_example_count = 0) do |reporter|
configuration.with_suite_hooks do
queue_runner.with_batch do |test_file_paths|
queue_runner.with_batch do |test_file_paths, store|
knapsack__load_spec_files_batch(test_file_paths)

examples_passed = ordering_strategy.order(world.example_groups).map do |example_group|
queue_runner.handle_signal!
example_group.run(reporter)
end.all?

node_examples_passed = false unless examples_passed
if examples_passed
store.last_batch_passed!
else
store.last_batch_failed!
node_examples_passed = false
end
3v0k4 marked this conversation as resolved.
Show resolved Hide resolved

knapsack__persist_example_statuses

Expand Down
8 changes: 7 additions & 1 deletion lib/knapsack_pro/runners/queue/base_runner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ def initialize(adapter_class)
def test_file_paths(args)
can_initialize_queue = args.fetch(:can_initialize_queue)
executed_test_files = args.fetch(:executed_test_files)
allocator.test_file_paths(can_initialize_queue, executed_test_files)
test_file_paths = allocator.test_file_paths(can_initialize_queue, executed_test_files)
store.add_batch(test_file_paths)
test_file_paths
end

def test_dir
Expand All @@ -38,6 +40,10 @@ def test_dir
attr_reader :allocator_builder,
:allocator

def store
KnapsackPro::Store::Server.client
end

def self.child_status
$?
end
Expand Down
13 changes: 12 additions & 1 deletion lib/knapsack_pro/runners/queue/cucumber_runner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ def self.run(args)
ENV['KNAPSACK_PRO_QUEUE_RECORDING_ENABLED'] = 'true'
ENV['KNAPSACK_PRO_QUEUE_ID'] = KnapsackPro::Config::EnvGenerator.set_queue_id

KnapsackPro::Store::Server.start

adapter_class = KnapsackPro::Adapters::CucumberAdapter
KnapsackPro::Config::Env.set_test_runner_adapter(adapter_class)
runner = new(adapter_class)
Expand Down Expand Up @@ -68,7 +70,12 @@ def self.run_tests(accumulator)
node_test_file_paths += test_file_paths

result_exitstatus = cucumber_run(runner, test_file_paths, args)
exitstatus = result_exitstatus if result_exitstatus != 0
if result_exitstatus == 0
store.last_batch_passed!
else
store.last_batch_failed!
exitstatus = result_exitstatus
end

# KnapsackPro::Hooks::Queue.call_after_subset_queue
# KnapsackPro::Report.save_subset_queue_to_file
Expand Down Expand Up @@ -111,6 +118,10 @@ def self.cucumber_run(runner, test_file_paths, args)

child_status.exitstatus
end

def self.store
KnapsackPro::Store::Server.client
end
end
end
end
Expand Down
13 changes: 12 additions & 1 deletion lib/knapsack_pro/runners/queue/minitest_runner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ def self.run(args)
ENV['KNAPSACK_PRO_QUEUE_RECORDING_ENABLED'] = 'true'
ENV['KNAPSACK_PRO_QUEUE_ID'] = KnapsackPro::Config::EnvGenerator.set_queue_id

KnapsackPro::Store::Server.start

adapter_class = KnapsackPro::Adapters::MinitestAdapter
KnapsackPro::Config::Env.set_test_runner_adapter(adapter_class)
runner = new(adapter_class)
Expand Down Expand Up @@ -79,7 +81,12 @@ def self.run_tests(accumulator)
node_test_file_paths += test_file_paths

result = minitest_run(runner, test_file_paths, args)
exitstatus = 1 unless result
if result
store.last_batch_passed!
else
store.last_batch_failed!
exitstatus = 1
end

KnapsackPro::Hooks::Queue.call_after_subset_queue

Expand Down Expand Up @@ -116,6 +123,10 @@ def self.minitest_run(runner, test_file_paths, args)

result
end

def self.store
KnapsackPro::Store::Server.client
end
end
end
end
Expand Down
8 changes: 7 additions & 1 deletion lib/knapsack_pro/runners/queue/rspec_runner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ def self.run(args, stream_error = $stderr, stream_out = $stdout)

ENV['KNAPSACK_PRO_TEST_SUITE_TOKEN'] = KnapsackPro::Config::Env.test_suite_token_rspec

KnapsackPro::Store::Server.start

rspec_pure = KnapsackPro::Pure::Queue::RSpecPure.new

queue_runner = new(KnapsackPro::Adapters::RSpecAdapter, rspec_pure, args, stream_error, stream_out)
Expand Down Expand Up @@ -84,7 +86,7 @@ def with_batch

KnapsackPro::Hooks::Queue.call_before_subset_queue

yield test_file_paths
yield test_file_paths, store

KnapsackPro::Hooks::Queue.call_after_subset_queue

Expand All @@ -101,6 +103,10 @@ def with_batch
end
end

def store
KnapsackPro::Store::Server.client
end

def handle_signal!
self.class.handle_signal!
end
Expand Down
19 changes: 19 additions & 0 deletions lib/knapsack_pro/store/client.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# frozen_string_literal: true

module KnapsackPro
module Store
# Consumers of this class are the gem's users.
# Ensure the API is backward compatible when introducing changes.
class Client
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def self.batches
client.batches
end

private

def self.client
KnapsackPro::Store::Server.client
end
end
end
end
26 changes: 26 additions & 0 deletions lib/knapsack_pro/store/queue_batch_manager.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# frozen_string_literal: true

module KnapsackPro
module Store
class QueueBatchManager
attr_reader :batches

def initialize
@batches = []
end

def add_batch(test_file_paths)
return if test_file_paths.empty?
@batches << KnapsackPro::Store::TestBatch.new(test_file_paths)
end

def last_batch_passed!
@batches.last.send(:passed!)
end

def last_batch_failed!
@batches.last.send(:failed!)
end
end
end
end
117 changes: 117 additions & 0 deletions lib/knapsack_pro/store/server.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
# frozen_string_literal: true

module KnapsackPro
module Store
class Server
extend Forwardable

def self.start
return unless @server_pid.nil?

assign_available_store_server_uri

@server_pid = fork do
queue = Thread::Queue.new

Signal.trap("TERM") {
queue.push(nil)
}

DRb.start_service(store_server_uri, new)
queue.pop
DRb.stop_service

# Wait for the drb server thread to finish before exiting.
DRb.thread&.join
end

::Kernel.at_exit do
stop
end
end

def self.client
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return @client unless @client.nil?

# must be called at least once per process
# https://ruby-doc.org/stdlib-2.7.0/libdoc/drb/rdoc/DRb.html
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do DRb.start_service(store_server_uri, KnapsackPro::Store::QueueBatchManager.new) instead and avoid the def_delegators, initialize, reset, and ping? That way we separate the Drb server logic from the manager logic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but I wanted to have a single DRb serivce (store) in a long term when we add more features to it (perhaps we would store data in memory instead of using json files). I prefer that instead of creating a new DRb service per feature.

I use the following architecture:

KnapsackPro::Store::Client - it exposes public methods that gem users can use. The API (public methods) should be stable so if we add new features they stay backward compatible.

KnapsackPro::Store::Sever - it's a DRb service that holds the data state in memory in a forked process (child process of the main process) so that we could share a state between processes like Knapsack process and Cucumber child process (Queue Mode).

KnapsackPro::Store::QueueBatchManager - a specific use case (a feature) . It holds a state of data for a given feature inside of the KnapsackPro::Store::Sever forked process.


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

def ping
true
end
3v0k4 marked this conversation as resolved.
Show resolved Hide resolved

private

def self.stop
return if @server_pid.nil?
Process.kill('TERM', @server_pid)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Process.waitpid2(@server_pid)
3v0k4 marked this conversation as resolved.
Show resolved Hide resolved
@server_pid = nil
rescue Errno::ESRCH # process does not exist
@server_pid = nil
end

def self.set_store_server_uri(uri)
ENV['KNAPSACK_PRO_STORE_SERVER_URI'] = uri
3v0k4 marked this conversation as resolved.
Show resolved Hide resolved
end

def self.store_server_uri
ENV['KNAPSACK_PRO_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
@assigned_store_server_uri ||=
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't @assigned_store_server_uri redundant? I think it's always true when ENV['KNAPSACK_PRO_STORE_SERVER_URI'] is set and always false when ENV['KNAPSACK_PRO_STORE_SERVER_URI'] is not set.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the code to d200ad9

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted it to the original code because it was causing flaky tests.

begin
find_available_drb_port_for_dummy_service
set_store_server_uri(DRb.uri)
stop_dummy_service
true
end
end

def self.find_available_drb_port_for_dummy_service
DRb.start_service('druby://localhost:0')
end

def self.stop_dummy_service
DRb.stop_service
end
end

class TestableServer < Server
def self.assign_available_store_server_uri
super
end

def self.reset
stop
set_store_server_uri(nil)
@assigned_store_server_uri = nil
@client = nil
end
end
end
end
35 changes: 35 additions & 0 deletions lib/knapsack_pro/store/test_batch.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# frozen_string_literal: true

module KnapsackPro
module Store
class TestBatch
BatchNotExecutedError = Class.new(StandardError)

attr_reader :test_file_paths

def initialize(test_file_paths)
@test_file_paths = test_file_paths
@passed = nil
end

def executed?
!@passed.nil?
end

def passed?
raise BatchNotExecutedError.new unless executed?
return @passed
end

private

def passed!
@passed = true
end

def failed!
@passed = false
end
end
end
end
Loading