From 88931f38642f4798b96f9a8ee44c25f09e299c3a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Fri, 27 Jan 2023 11:15:34 +0100
Subject: [PATCH] ci: Include test files filtering in Knapsack allocation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Rémy Coutable <remy@rymai.me>
---
scripts/rspec_helpers.sh | 10 +-
.../lib/tooling/parallel_rspec_runner_spec.rb | 96 +++++++++---------
tooling/lib/tooling/parallel_rspec_runner.rb | 99 +++++++++++++------
3 files changed, 119 insertions(+), 86 deletions(-)
diff --git a/scripts/rspec_helpers.sh b/scripts/rspec_helpers.sh
index c6ec773cf7802..729892ec84e33 100644
--- a/scripts/rspec_helpers.sh
+++ b/scripts/rspec_helpers.sh
@@ -137,6 +137,9 @@ function debug_rspec_variables() {
echoinfo "SKIPPED_FLAKY_TESTS_REPORT_PATH: ${SKIPPED_FLAKY_TESTS_REPORT_PATH}"
echoinfo "CRYSTALBALL: ${CRYSTALBALL:-}"
+
+ echoinfo "RSPEC_TESTS_MAPPING_ENABLED: ${RSPEC_TESTS_MAPPING_ENABLED:-}"
+ echoinfo "RSPEC_TESTS_FILTER_FILE: ${RSPEC_TESTS_FILTER_FILE:-}"
}
function handle_retry_rspec_in_new_process() {
@@ -194,12 +197,7 @@ function rspec_paralellized_job() {
cp "${KNAPSACK_RSPEC_SUITE_REPORT_PATH}" "${KNAPSACK_REPORT_PATH}"
- export KNAPSACK_TEST_FILE_PATTERN="spec/{,**/}*_spec.rb"
-
- if [[ "${test_level}" != "foss-impact" ]]; then
- export KNAPSACK_TEST_FILE_PATTERN=$(ruby -r./tooling/quality/test_level.rb -e "puts Quality::TestLevel.new(${spec_folder_prefixes}).pattern(:${test_level})")
- fi
-
+ export KNAPSACK_TEST_FILE_PATTERN=$(ruby -r./tooling/quality/test_level.rb -e "puts Quality::TestLevel.new(${spec_folder_prefixes}).pattern(:${test_level})")
export FLAKY_RSPEC_REPORT_PATH="${rspec_flaky_folder_path}all_${report_name}_report.json"
export NEW_FLAKY_RSPEC_REPORT_PATH="${rspec_flaky_folder_path}new_${report_name}_report.json"
export SKIPPED_FLAKY_TESTS_REPORT_PATH="${rspec_flaky_folder_path}skipped_flaky_tests_${report_name}_report.txt"
diff --git a/spec/tooling/lib/tooling/parallel_rspec_runner_spec.rb b/spec/tooling/lib/tooling/parallel_rspec_runner_spec.rb
index 4b44d991d89b9..b7b39c3781933 100644
--- a/spec/tooling/lib/tooling/parallel_rspec_runner_spec.rb
+++ b/spec/tooling/lib/tooling/parallel_rspec_runner_spec.rb
@@ -1,90 +1,84 @@
# frozen_string_literal: true
+require 'tempfile'
+
require_relative '../../../../tooling/lib/tooling/parallel_rspec_runner'
RSpec.describe Tooling::ParallelRSpecRunner do # rubocop:disable RSpec/FilePath
describe '#run' do
- let(:allocator) { instance_double(Knapsack::Allocator) }
- let(:rspec_args) { '--seed 123' }
- let(:filter_tests_file) { 'tests.txt' }
- let(:node_tests) { %w[01_spec.rb 03_spec.rb 05_spec.rb] }
- let(:filter_tests) { '01_spec.rb 02_spec.rb 03_spec.rb' }
let(:test_dir) { 'spec' }
+ let(:node_tests) { %w[01_spec.rb 03_spec.rb] }
+ let(:allocator) { instance_double(Knapsack::Allocator, test_dir: test_dir, node_tests: node_tests) }
+ let(:allocator_builder) { double(Knapsack::AllocatorBuilder, allocator: allocator) } # rubocop:disable RSpec/VerifiedDoubles
+
+ let(:filter_tests) { [] }
+ let(:filter_tests_file) { nil }
+ let(:filter_tests_file_path) { nil }
before do
+ allow(Knapsack::AllocatorBuilder).to receive(:new).and_return(allocator_builder)
allow(Knapsack.logger).to receive(:info)
- allow(allocator).to receive(:node_tests).and_return(node_tests)
- allow(allocator).to receive(:test_dir).and_return(test_dir)
- allow(File).to receive(:exist?).with(filter_tests_file).and_return(true)
- allow(File).to receive(:read).and_call_original
- allow(File).to receive(:read).with(filter_tests_file).and_return(filter_tests)
- allow(subject).to receive(:exec)
end
- subject { described_class.new(allocator: allocator, filter_tests_file: filter_tests_file, rspec_args: rspec_args) }
-
- shared_examples 'runs node tests' do
- it 'runs rspec with tests allocated for this node' do
- expect_command(%w[bundle exec rspec --seed 123 --default-path spec -- 01_spec.rb 03_spec.rb 05_spec.rb])
-
- subject.run
+ after do
+ if filter_tests_file.respond_to?(:close)
+ filter_tests_file.close
+ File.unlink(filter_tests_file)
end
end
- context 'given filter tests' do
- it 'reads filter tests file for list of tests' do
- expect(File).to receive(:read).with(filter_tests_file)
+ subject { described_class.new(filter_tests_file: filter_tests_file_path, rspec_args: rspec_args) }
- subject.run
- end
+ shared_examples 'runs node tests' do
+ let(:rspec_args) { nil }
- it 'runs rspec filter tests that are allocated for this node' do
- expect_command(%w[bundle exec rspec --seed 123 --default-path spec -- 01_spec.rb 03_spec.rb])
+ it 'runs rspec with tests allocated for this node' do
+ expect(allocator_builder).to receive(:filter_tests=).with(filter_tests)
+ expect_command(%W[bundle exec rspec#{rspec_args} --] + node_tests)
subject.run
end
-
- context 'when there is no intersect between allocated tests and filtered tests' do
- let(:filter_tests) { '99_spec.rb' }
-
- it 'does not run rspec' do
- expect(subject).not_to receive(:exec)
-
- subject.run
- end
- end
end
- context 'with empty filter tests file' do
- let(:filter_tests) { '' }
+ context 'without filter_tests_file option' do
+ subject { described_class.new(rspec_args: rspec_args) }
it_behaves_like 'runs node tests'
end
- context 'without filter_tests_file option' do
- let(:filter_tests_file) { nil }
+ context 'given filter tests file' do
+ let(:filter_tests_file) do
+ Tempfile.create.tap do |f| # rubocop:disable Rails/SaveBang
+ f.write(filter_tests.join(' '))
+ f.rewind
+ end
+ end
- it_behaves_like 'runs node tests'
- end
+ let(:filter_tests_file_path) { filter_tests_file.path }
- context 'if filter_tests_file does not exist' do
- before do
- allow(File).to receive(:exist?).with(filter_tests_file).and_return(false)
+ context 'when filter_tests_file is empty' do
+ it_behaves_like 'runs node tests'
end
- it_behaves_like 'runs node tests'
- end
+ context 'when filter_tests_file does not exist' do
+ let(:filter_tests_file_path) { 'doesnt_exist' }
- context 'without rspec args' do
- let(:rspec_args) { nil }
+ it_behaves_like 'runs node tests'
+ end
- it 'runs rspec with without extra arguments' do
- expect_command(%w[bundle exec rspec --default-path spec -- 01_spec.rb 03_spec.rb])
+ context 'when filter_tests_file is not empty' do
+ let(:filter_tests) { %w[01_spec.rb 02_spec.rb 03_spec.rb] }
- subject.run
+ it_behaves_like 'runs node tests'
end
end
+ context 'with rspec args' do
+ let(:rspec_args) { ' --seed 123' }
+
+ it_behaves_like 'runs node tests'
+ end
+
def expect_command(cmd)
expect(subject).to receive(:exec).with(*cmd)
end
diff --git a/tooling/lib/tooling/parallel_rspec_runner.rb b/tooling/lib/tooling/parallel_rspec_runner.rb
index b1ddc91e831e3..834d9ec23a75d 100644
--- a/tooling/lib/tooling/parallel_rspec_runner.rb
+++ b/tooling/lib/tooling/parallel_rspec_runner.rb
@@ -1,6 +1,60 @@
# frozen_string_literal: true
require 'knapsack'
+require 'fileutils'
+
+module Knapsack
+ module Distributors
+ class BaseDistributor
+ # Refine https://github.com/KnapsackPro/knapsack/blob/v1.21.1/lib/knapsack/distributors/base_distributor.rb
+ # to take in account the additional filtering we do for predictive jobs.
+ module BaseDistributorWithTestFiltering
+ attr_reader :filter_tests
+
+ def initialize(args = {})
+ super
+
+ @filter_tests = args[:filter_tests]
+ end
+
+ def all_tests
+ @all_tests ||= begin
+ pattern_tests = Dir.glob(test_file_pattern).uniq
+
+ if filter_tests.empty?
+ pattern_tests
+ else
+ pattern_tests & filter_tests
+ end
+ end.sort
+ end
+ end
+
+ prepend BaseDistributorWithTestFiltering
+ end
+ end
+
+ class AllocatorBuilder
+ # Refine https://github.com/KnapsackPro/knapsack/blob/v1.21.1/lib/knapsack/allocator_builder.rb
+ # to take in account the additional filtering we do for predictive jobs.
+ module AllocatorBuilderWithTestFiltering
+ attr_accessor :filter_tests
+
+ def allocator
+ Knapsack::Allocator.new({
+ report: Knapsack.report.open,
+ test_file_pattern: test_file_pattern,
+ ci_node_total: Knapsack::Config::Env.ci_node_total,
+ ci_node_index: Knapsack::Config::Env.ci_node_index,
+ # Additional argument
+ filter_tests: filter_tests
+ })
+ end
+ end
+
+ prepend AllocatorBuilderWithTestFiltering
+ end
+end
# A custom parallel rspec runner based on Knapsack runner
# which takes in additional option for a file containing
@@ -13,32 +67,26 @@
# would be executed in the CI node.
#
# Reference:
-# https://github.com/ArturT/knapsack/blob/v1.20.0/lib/knapsack/runners/rspec_runner.rb
+# https://github.com/ArturT/knapsack/blob/v1.21.1/lib/knapsack/runners/rspec_runner.rb
module Tooling
class ParallelRSpecRunner
def self.run(rspec_args: nil, filter_tests_file: nil)
new(rspec_args: rspec_args, filter_tests_file: filter_tests_file).run
end
- def initialize(allocator: knapsack_allocator, filter_tests_file: nil, rspec_args: nil)
- @allocator = allocator
+ def initialize(filter_tests_file: nil, rspec_args: nil)
@filter_tests_file = filter_tests_file
@rspec_args = rspec_args&.split(' ') || []
end
def run
- Knapsack.logger.info
- Knapsack.logger.info 'Knapsack node specs:'
- Knapsack.logger.info node_tests
- Knapsack.logger.info
- Knapsack.logger.info 'Filter specs:'
- Knapsack.logger.info filter_tests
- Knapsack.logger.info
- Knapsack.logger.info 'Running specs:'
- Knapsack.logger.info tests_to_run
- Knapsack.logger.info
-
- if tests_to_run.empty?
+ if ENV['KNAPSACK_RSPEC_SUITE_REPORT_PATH']
+ knapsack_dir = File.dirname(ENV['KNAPSACK_RSPEC_SUITE_REPORT_PATH'])
+ FileUtils.mkdir_p(knapsack_dir)
+ File.write(File.join(knapsack_dir, 'node_specs.txt'), node_tests.join("\n"))
+ end
+
+ if node_tests.empty?
Knapsack.logger.info 'No tests to run on this node, exiting.'
return
end
@@ -50,26 +98,16 @@ def run
private
- attr_reader :allocator, :filter_tests_file, :rspec_args
+ attr_reader :filter_tests_file, :rspec_args
def rspec_command
%w[bundle exec rspec].tap do |cmd|
cmd.push(*rspec_args)
- cmd.push('--default-path', allocator.test_dir)
cmd.push('--')
- cmd.push(*tests_to_run)
+ cmd.push(*node_tests)
end
end
- def tests_to_run
- if filter_tests.empty?
- Knapsack.logger.info 'Running all node tests without filter'
- return node_tests
- end
-
- @tests_to_run ||= node_tests & filter_tests
- end
-
def node_tests
allocator.node_tests
end
@@ -85,8 +123,11 @@ def tests_from_file(filter_tests_file)
File.read(filter_tests_file).split(' ')
end
- def knapsack_allocator
- Knapsack::AllocatorBuilder.new(Knapsack::Adapters::RSpecAdapter).allocator
+ def allocator
+ @allocator ||=
+ Knapsack::AllocatorBuilder.new(Knapsack::Adapters::RSpecAdapter).tap do |builder|
+ builder.filter_tests = filter_tests
+ end.allocator
end
end
end
--
GitLab