Use fixed test suite split by default

CHANGELOG.md

@@ -2,6 +2,32 @@
 
 * TODO
 
+### 0.10.0
+
+* Add new environment variable `KNAPSACK_PRO_FIXED_TEST_SUITE_SPLIT`. The default value is true.
+
+    It means when you run test suite again for the same commit hash and total number of nodes and for the same branch
+    then you will get exactly the same test suite split.
+    This is the new default behavior for the knapsack_pro gem. Thanks to that when tests on one of your node failed
+    you can retry the node with exactly the same subset of tests that were run on the node in the first place.
+
+    There is one edge case. When you run tests for the first time and there is no data collected about time execution of your tests then
+    we need to collect data to prepare the first test suite split. The second run of your tests will have fixed test suite split.
+    To compare if all your CI nodes are running based on the same test suite split seed you can check the value for seed in knapsack logging message
+    before your test starts. The message looks like:
+
+        [knapsack_pro] Test suite split seed: 8a606431-02a1-4766-9878-0ea42a07ad21
+
+  * Show test suite split seed in logger based on `build_distribution_id` from Knapsack Pro API.
+  * Send `fixed_test_suite_split` param to build distribution Knapsack Pro API endpoint.
+
+  Related issues:
+
+  * https://github.com/KnapsackPro/knapsack_pro-ruby/issues/15
+  * https://github.com/KnapsackPro/knapsack_pro-ruby/issues/12
+
+https://github.com/KnapsackPro/knapsack_pro-ruby/compare/v0.9.0...v0.10.0
+
 ### 0.9.0
 
 * Add https support for Knapsack Pro API endpoint

README.md

@@ -68,6 +68,7 @@ For instance when you will run tests with rake knapsack_pro:rspec then:
     - [When you set global variable `KNAPSACK_PRO_REPOSITORY_ADAPTER=git` (required when CI provider is not supported)](#when-you-set-global-variable-knapsack_pro_repository_adaptergit-required-when-ci-provider-is-not-supported)
 - [Extra configuration for CI server](#extra-configuration-for-ci-server)
   - [Info about ENV variables](#info-about-env-variables)
+    - [KNAPSACK_PRO_FIXED_TEST_SUITE_SPLITE (test suite split based on seed)](#knapsack_pro_fixed_test_suite_splite-test-suite-split-based-on-seed)
     - [Environment variables for debugging gem](#environment-variables-for-debugging-gem)
   - [Passing arguments to rake task](#passing-arguments-to-rake-task)
     - [Passing arguments to rspec](#passing-arguments-to-rspec)
@@ -268,6 +269,38 @@ In case when you use other CI provider for instance [Jenkins](https://jenkins-ci
 
 `KNAPSACK_PRO_CI_NODE_INDEX` - index of current CI node starts from 0. Second CI node should have `KNAPSACK_PRO_CI_NODE_INDEX=1`.
 
+#### KNAPSACK_PRO_FIXED_TEST_SUITE_SPLITE (test suite split based on seed)
+
+* `KNAPSACK_PRO_FIXED_TEST_SUITE_SPLIT=true` (default)
+
+    It means when you run test suite again for the same commit hash and total number of nodes and for the same branch
+    then you will get exactly the same test suite split.
+
+    Thanks to that when tests on one of your node failed you can retry the node with exactly the same subset of tests that were run on the node in the first place.
+
+    There is one edge case. When you run tests for the first time and there is no data collected about time execution of your tests then
+    we need to collect data to prepare the first test suite split. The second run of your tests will have fixed test suite split.
+
+    To compare if all your CI nodes are running based on the same test suite split seed you can check the value for seed in knapsack logging message
+    before your test starts. The message looks like:
+
+        [knapsack_pro] Test suite split seed: 8a606431-02a1-4766-9878-0ea42a07ad21
+
+* `KNAPSACK_PRO_FIXED_TEST_SUITE_SPLIT=false`
+
+    When you disable fixed test suite split then your will get test suite split based on most up to date data about your test suite time execution.
+    For instance, when you run tests for the second time for the same commit hash then your will get more optimal test suite split than it was on the first run.
+
+    Don't disable fixed test suite split when:
+
+    * you expect to run the same subset of test suite multiple times for the same node (for instance your would like to retry only single CI node that failed)
+
+        Example of issue: https://github.com/KnapsackPro/knapsack_pro-ruby/issues/15
+
+    * you start your tests not at the same time across your CI nodes. For instance, one of the CI node finished faster than the other CI node started. This would change the seed for the second CI node that started later.
+
+        Example of issue: https://github.com/KnapsackPro/knapsack_pro-ruby/issues/12
+
 #### Environment variables for debugging gem
 
 `KNAPSACK_PRO_ENDPOINT` - Default value is `http://api.knapsackpro.com` which is endpoint for [Knapsack Pro API](http://docs.knapsackpro.com).

lib/knapsack_pro/client/api/v1/build_distributions.rb

@@ -9,6 +9,7 @@ def subset(args)
                 endpoint_path: '/v1/build_distributions/subset',
                 http_method: :post,
                 request_hash: {
+                  :fixed_test_suite_split => KnapsackPro::Config::Env.fixed_test_suite_split,
                   :commit_hash => args.fetch(:commit_hash),
                   :branch => args.fetch(:branch),
                   :node_total => args.fetch(:node_total),

lib/knapsack_pro/client/connection.rb

@@ -65,6 +65,15 @@ def parse_response(body)
         nil
       end
 
+      def seed
+        return if @response.nil? || @response == ''
+        response['build_distribution_id']
+      end
+
+      def has_seed?
+        !seed.nil?
+      end
+
       def post
         uri = URI.parse(endpoint_url)
         http = Net::HTTP.new(uri.host, uri.port)
@@ -78,6 +87,7 @@ def post
         request_uuid = http_response.header['X-Request-Id']
 
         logger.info("API request UUID: #{request_uuid}")
+        logger.info("Test suite split seed: #{seed}") if has_seed?
         logger.info('API response:')
         if errors?
           logger.error(response)

lib/knapsack_pro/config/env.rb

@@ -61,6 +61,10 @@ def endpoint
           end
         end
 
+        def fixed_test_suite_split
+          ENV.fetch('KNAPSACK_PRO_FIXED_TEST_SUITE_SPLIT', true)
+        end
+
         def test_suite_token
           required_env('KNAPSACK_PRO_TEST_SUITE_TOKEN')
         end

spec/knapsack_pro/client/api/v1/build_distributions_spec.rb

@@ -1,5 +1,6 @@
 describe KnapsackPro::Client::API::V1::BuildDistributions do
   describe '.subset' do
+    let(:fixed_test_suite_split) { double }
     let(:commit_hash) { double }
     let(:branch) { double }
     let(:node_total) { double }
@@ -16,12 +17,17 @@
       )
     end
 
+    before do
+      expect(KnapsackPro::Config::Env).to receive(:fixed_test_suite_split).and_return(fixed_test_suite_split)
+    end
+
     it do
       action = double
       expect(KnapsackPro::Client::API::Action).to receive(:new).with({
         endpoint_path: '/v1/build_distributions/subset',
         http_method: :post,
         request_hash: {
+          fixed_test_suite_split: fixed_test_suite_split,
           commit_hash: commit_hash,
           branch: branch,
           node_total: node_total,

spec/knapsack_pro/client/connection_spec.rb

@@ -40,15 +40,17 @@
           "{\"fake\":\"hash\",\"test_suite_token\":\"3fa64859337f6e56409d49f865d13fd7\"}",
           { "Content-Type" => "application/json", "Accept" => "application/json" }
         ).and_return(http_response)
-
-        expect(KnapsackPro).to receive(:logger).exactly(3).and_return(logger)
-        expect(logger).to receive(:info).with('API request UUID: fake-uuid')
-        expect(logger).to receive(:info).with('API response:')
       end
 
       context 'when body response is json' do
         let(:body) { '{"errors": "value"}' }
 
+        before do
+          expect(KnapsackPro).to receive(:logger).exactly(3).and_return(logger)
+          expect(logger).to receive(:info).with('API request UUID: fake-uuid')
+          expect(logger).to receive(:info).with('API response:')
+        end
+
         it do
           parsed_response = { 'errors' => 'value' }
 
@@ -60,9 +62,36 @@
         end
       end
 
+      context 'when body response is json with build_distribution_id' do
+        let(:body) { '{"build_distribution_id": "seed-uuid"}' }
+
+        before do
+          expect(KnapsackPro).to receive(:logger).exactly(4).and_return(logger)
+          expect(logger).to receive(:info).with('API request UUID: fake-uuid')
+          expect(logger).to receive(:info).with("Test suite split seed: seed-uuid")
+          expect(logger).to receive(:info).with('API response:')
+        end
+
+        it do
+          parsed_response = { 'build_distribution_id' => 'seed-uuid' }
+
+          expect(logger).to receive(:info).with(parsed_response)
+
+          expect(subject).to eq(parsed_response)
+          expect(connection.success?).to be true
+          expect(connection.errors?).to be false
+        end
+      end
+
       context 'when body response is empty' do
         let(:body) { '' }
 
+        before do
+          expect(KnapsackPro).to receive(:logger).exactly(3).and_return(logger)
+          expect(logger).to receive(:info).with('API request UUID: fake-uuid')
+          expect(logger).to receive(:info).with('API response:')
+        end
+
         it do
           expect(logger).to receive(:info).with('')
 

spec/knapsack_pro/config/env_spec.rb

@@ -219,6 +219,20 @@
     end
   end
 
+  describe '.fixed_test_suite_split' do
+    subject { described_class.fixed_test_suite_split }
+
+    context 'when ENV exists' do
+      before { stub_const("ENV", { 'KNAPSACK_PRO_FIXED_TEST_SUITE_SPLIT' => false }) }
+      it { should eq false }
+    end
+
+    context "when ENV doesn't exist" do
+      before { stub_const("ENV", {}) }
+      it { should be true }
+    end
+  end
+
   describe '.test_suite_token' do
     subject { described_class.test_suite_token }