From 0b5045fe128739e5ae2239b147c2e2ad3d6fc148 Mon Sep 17 00:00:00 2001 From: Andrey Date: Wed, 22 May 2024 12:21:59 +0200 Subject: [PATCH 1/5] fix knapsack instrumentation when RSpec Runner is already patched by the time Datadog.configure is invoked --- .../contrib/rspec/knapsack_pro/extension.rb | 1 - .../ci/contrib/rspec/knapsack_pro/patcher.rb | 26 +++++++++++++++++++ lib/datadog/ci/contrib/rspec/patcher.rb | 10 ++++--- .../ci/contrib/rspec/knapsack_pro/patcher.rbs | 13 ++++++++++ vendor/rbs/knapsack_pro/0/knapsack_pro.rbs | 3 +++ 5 files changed, 48 insertions(+), 5 deletions(-) create mode 100644 lib/datadog/ci/contrib/rspec/knapsack_pro/patcher.rb create mode 100644 sig/datadog/ci/contrib/rspec/knapsack_pro/patcher.rbs diff --git a/lib/datadog/ci/contrib/rspec/knapsack_pro/extension.rb b/lib/datadog/ci/contrib/rspec/knapsack_pro/extension.rb index bfe75453..f7fe2dcd 100644 --- a/lib/datadog/ci/contrib/rspec/knapsack_pro/extension.rb +++ b/lib/datadog/ci/contrib/rspec/knapsack_pro/extension.rb @@ -8,7 +8,6 @@ module Datadog module CI module Contrib module RSpec - # Instrument RSpec::Core::Example module KnapsackPro module Extension def self.included(base) diff --git a/lib/datadog/ci/contrib/rspec/knapsack_pro/patcher.rb b/lib/datadog/ci/contrib/rspec/knapsack_pro/patcher.rb new file mode 100644 index 00000000..b66ac1c6 --- /dev/null +++ b/lib/datadog/ci/contrib/rspec/knapsack_pro/patcher.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +module Datadog + module CI + module Contrib + module RSpec + module KnapsackPro + module Patcher + def self.patch + if defined?(::KnapsackPro::Extensions::RSpecExtension::Runner) && + ::RSpec::Core::Runner.ancestors.include?(::KnapsackPro::Extensions::RSpecExtension::Runner) + # knapsack already patched rspec runner + require_relative "runner" + ::RSpec::Core::Runner.include(Datadog::CI::Contrib::RSpec::KnapsackPro::Runner) + else + # knapsack didn't patch rspec runner yet + require_relative "extension" + ::KnapsackPro::Extensions::RSpecExtension.include(KnapsackPro::Extension) + end + end + end + end + end + end + end +end diff --git a/lib/datadog/ci/contrib/rspec/patcher.rb b/lib/datadog/ci/contrib/rspec/patcher.rb index 41e6935d..8fec2cb1 100644 --- a/lib/datadog/ci/contrib/rspec/patcher.rb +++ b/lib/datadog/ci/contrib/rspec/patcher.rb @@ -27,14 +27,16 @@ def patch ::RSpec::Queue::Runner.include(Runner) end - # Knapsack Pro test runner instrumentation - # https://github.com/KnapsackPro/knapsack_pro-ruby if knapsack_pro? - require_relative "knapsack_pro/extension" - ::KnapsackPro::Extensions::RSpecExtension.include(KnapsackPro::Extension) + # Knapsack Pro test runner instrumentation + # https://github.com/KnapsackPro/knapsack_pro-ruby + require_relative "knapsack_pro/patcher" + Datadog::CI::Contrib::RSpec::KnapsackPro::Patcher.patch end + # default rspec test runner instrumentation ::RSpec::Core::Runner.include(Runner) + ::RSpec::Core::Example.include(Example) ::RSpec::Core::ExampleGroup.include(ExampleGroup) end diff --git a/sig/datadog/ci/contrib/rspec/knapsack_pro/patcher.rbs b/sig/datadog/ci/contrib/rspec/knapsack_pro/patcher.rbs new file mode 100644 index 00000000..11d34d86 --- /dev/null +++ b/sig/datadog/ci/contrib/rspec/knapsack_pro/patcher.rbs @@ -0,0 +1,13 @@ +module Datadog + module CI + module Contrib + module RSpec + module KnapsackPro + module Patcher + def self.patch: () -> void + end + end + end + end + end +end diff --git a/vendor/rbs/knapsack_pro/0/knapsack_pro.rbs b/vendor/rbs/knapsack_pro/0/knapsack_pro.rbs index 58696d5a..d44f12b6 100644 --- a/vendor/rbs/knapsack_pro/0/knapsack_pro.rbs +++ b/vendor/rbs/knapsack_pro/0/knapsack_pro.rbs @@ -12,6 +12,9 @@ module KnapsackPro module Extensions module RSpecExtension def setup!: () -> void + + module Runner + end end end end \ No newline at end of file From 984e1f41e2a48c6d1885eef7d3e0dee10460ceda Mon Sep 17 00:00:00 2001 From: Andrey Date: Wed, 22 May 2024 13:36:02 +0200 Subject: [PATCH 2/5] do not instrument RSpec session with --dry-run option --- lib/datadog/ci/contrib/rspec/example.rb | 1 + lib/datadog/ci/contrib/rspec/example_group.rb | 1 + .../ci/contrib/rspec/knapsack_pro/runner.rb | 1 + lib/datadog/ci/contrib/rspec/runner.rb | 1 + .../knapsack_rspec/instrumentation_spec.rb | 22 ++++++++++++++++ .../ci/contrib/rspec/instrumentation_spec.rb | 25 ++++++++++++++++--- vendor/rbs/rspec/0/rspec.rbs | 5 ++++ 7 files changed, 53 insertions(+), 3 deletions(-) diff --git a/lib/datadog/ci/contrib/rspec/example.rb b/lib/datadog/ci/contrib/rspec/example.rb index fa6d3c90..53a2a624 100644 --- a/lib/datadog/ci/contrib/rspec/example.rb +++ b/lib/datadog/ci/contrib/rspec/example.rb @@ -17,6 +17,7 @@ def self.included(base) module InstanceMethods def run(*) + return super if ::RSpec.configuration.dry_run? return super unless datadog_configuration[:enabled] test_name = full_description.strip diff --git a/lib/datadog/ci/contrib/rspec/example_group.rb b/lib/datadog/ci/contrib/rspec/example_group.rb index b13f2173..0a2680fb 100644 --- a/lib/datadog/ci/contrib/rspec/example_group.rb +++ b/lib/datadog/ci/contrib/rspec/example_group.rb @@ -16,6 +16,7 @@ def self.included(base) # Instance methods for configuration module ClassMethods def run(reporter = ::RSpec::Core::NullReporter) + return super if ::RSpec.configuration.dry_run? return super unless datadog_configuration[:enabled] return super unless top_level? diff --git a/lib/datadog/ci/contrib/rspec/knapsack_pro/runner.rb b/lib/datadog/ci/contrib/rspec/knapsack_pro/runner.rb index 9a5ec11a..59cbe98f 100644 --- a/lib/datadog/ci/contrib/rspec/knapsack_pro/runner.rb +++ b/lib/datadog/ci/contrib/rspec/knapsack_pro/runner.rb @@ -15,6 +15,7 @@ def self.included(base) module InstanceMethods def knapsack__run_specs(*) + return super if ::RSpec.configuration.dry_run? return super unless datadog_configuration[:enabled] test_session = CI.start_test_session( diff --git a/lib/datadog/ci/contrib/rspec/runner.rb b/lib/datadog/ci/contrib/rspec/runner.rb index 29d4c692..2a52fac4 100644 --- a/lib/datadog/ci/contrib/rspec/runner.rb +++ b/lib/datadog/ci/contrib/rspec/runner.rb @@ -15,6 +15,7 @@ def self.included(base) module InstanceMethods def run_specs(*) + return super if ::RSpec.configuration.dry_run? return super unless datadog_configuration[:enabled] test_session = CI.start_test_session( diff --git a/spec/datadog/ci/contrib/knapsack_rspec/instrumentation_spec.rb b/spec/datadog/ci/contrib/knapsack_rspec/instrumentation_spec.rb index 01dea690..c71a05bb 100644 --- a/spec/datadog/ci/contrib/knapsack_rspec/instrumentation_spec.rb +++ b/spec/datadog/ci/contrib/knapsack_rspec/instrumentation_spec.rb @@ -75,4 +75,26 @@ def devnull expect(test_spans).to all have_test_tag(:test_module_id) expect(test_spans).to all have_test_tag(:test_session_id) end + + context "when collecting examples with knapsack_pro:rspec_test_example_detector" do + it "does not instrument this rspec session" do + with_new_rspec_environment do + ClimateControl.modify( + "KNAPSACK_PRO_CI_NODE_BUILD_ID" => "142", + "KNAPSACK_PRO_TEST_SUITE_TOKEN_RSPEC" => "example_token", + "KNAPSACK_PRO_FIXED_QUEUE_SPLIT" => "true" + ) do + expect(KnapsackPro::SlowTestFileDeterminer).to receive(:read_from_json_report).and_return([]) + + detector = KnapsackPro::TestCaseDetectors::RSpecTestExampleDetector.new + detector.generate_json_report + rescue ArgumentError + # suppress invalid API key error + end + end + + expect(test_session_span).to be_nil + expect(test_spans).to be_empty + end + end end diff --git a/spec/datadog/ci/contrib/rspec/instrumentation_spec.rb b/spec/datadog/ci/contrib/rspec/instrumentation_spec.rb index d52c6bb1..e7cac9f5 100644 --- a/spec/datadog/ci/contrib/rspec/instrumentation_spec.rb +++ b/spec/datadog/ci/contrib/rspec/instrumentation_spec.rb @@ -27,7 +27,8 @@ def rspec_session_run( test: false, context: false, suite: false - } + }, + dry_run: false ) test_meta = unskippable[:test] ? {Datadog::CI::Ext::Test::ITR_UNSKIPPABLE_OPTION => true} : {} context_meta = unskippable[:context] ? {Datadog::CI::Ext::Test::ITR_UNSKIPPABLE_OPTION => true} : {} @@ -58,7 +59,11 @@ def rspec_session_run( end end - options = ::RSpec::Core::ConfigurationOptions.new(%w[--pattern none]) + options_array = %w[--pattern none] + if dry_run + options_array << "--dry-run" + end + options = ::RSpec::Core::ConfigurationOptions.new(options_array) ::RSpec::Core::Runner.new(options).run(devnull, devnull) spec @@ -104,7 +109,7 @@ def rspec_session_run( :source_file, "spec/datadog/ci/contrib/rspec/instrumentation_spec.rb" ) - expect(first_test_span).to have_test_tag(:source_start, "77") + expect(first_test_span).to have_test_tag(:source_start, "82") expect(first_test_span).to have_test_tag( :codeowners, "[\"@DataDog/ruby-guild\", \"@DataDog/ci-app-libraries\"]" @@ -773,4 +778,18 @@ def rspec_skipped_session_run end end end + + context "with dry run" do + include_context "CI mode activated" do + let(:integration_name) { :rspec } + let(:integration_options) { {service_name: "lspec"} } + end + + it "does not instrument test session" do + rspec_session_run(dry_run: true) + + expect(test_session_span).to be_nil + expect(test_spans).to be_empty + end + end end diff --git a/vendor/rbs/rspec/0/rspec.rbs b/vendor/rbs/rspec/0/rspec.rbs index 5ccb0357..d08273cc 100644 --- a/vendor/rbs/rspec/0/rspec.rbs +++ b/vendor/rbs/rspec/0/rspec.rbs @@ -1,4 +1,5 @@ module RSpec + def self.configuration: () -> RSpec::Core::Configuration end module RSpec::Core @@ -36,3 +37,7 @@ end class RSpec::Core::NullReporter end + +class RSpec::Core::Configuration + def dry_run?: () -> bool +end From 802ba1195305c489d08de58a4d2e3c83a9baa233 Mon Sep 17 00:00:00 2001 From: Andrey Date: Wed, 22 May 2024 15:39:53 +0200 Subject: [PATCH 3/5] add unit test for the case when datadog-ci is configured when knapsack_pro runner is already running (like in rspec_go rake task) --- Rakefile | 8 +- .../knapsack_rspec/instrumentation_spec.rb | 4 +- .../knapsack_rspec_go/instrumentation_spec.rb | 79 +++++++++++++++++++ .../suite_under_test/some_test_rspec.rb | 13 +++ spec/knapsack_helper.rb | 8 ++ spec/support/contexts/ci_mode.rb | 4 + 6 files changed, 111 insertions(+), 5 deletions(-) create mode 100644 spec/datadog/ci/contrib/knapsack_rspec_go/instrumentation_spec.rb create mode 100644 spec/datadog/ci/contrib/knapsack_rspec_go/suite_under_test/some_test_rspec.rb create mode 100644 spec/knapsack_helper.rb diff --git a/Rakefile b/Rakefile index 79563a18..bcf0c33a 100644 --- a/Rakefile +++ b/Rakefile @@ -60,7 +60,10 @@ TEST_METADATA = { "minitest-5-shoulda-context-2-shoulda-matchers-6" => "❌ 2.7 / ❌ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ 3.3 / ✅ jruby" }, "knapsack_rspec" => { - "knapsack_pro-7-rspec-3" => "✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ 3.3 / ✅ jruby" + "knapsack_pro-7-rspec-3" => "✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ 3.3 / ❌ jruby" + }, + "knapsack_rspec_go" => { + "knapsack_pro-7-rspec-3" => "✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ 3.3 / ❌ jruby" } } @@ -132,7 +135,8 @@ namespace :spec do :activesupport, :ci_queue_minitest, :ci_queue_rspec, - :knapsack_rspec + :knapsack_rspec, + :knapsack_rspec_go ].each do |contrib| desc "" # "Explicitly hiding from `rake -T`" RSpec::Core::RakeTask.new(contrib) do |t, args| diff --git a/spec/datadog/ci/contrib/knapsack_rspec/instrumentation_spec.rb b/spec/datadog/ci/contrib/knapsack_rspec/instrumentation_spec.rb index c71a05bb..a631e1f8 100644 --- a/spec/datadog/ci/contrib/knapsack_rspec/instrumentation_spec.rb +++ b/spec/datadog/ci/contrib/knapsack_rspec/instrumentation_spec.rb @@ -2,8 +2,6 @@ require "fileutils" RSpec.describe "RSpec instrumentation with Knapsack Pro runner in queue mode" do - before { skip("jruby fails on file with emojis in name") if PlatformHelpers.jruby? } - include_context "CI mode activated" do let(:integration_name) { :rspec } end @@ -80,7 +78,7 @@ def devnull it "does not instrument this rspec session" do with_new_rspec_environment do ClimateControl.modify( - "KNAPSACK_PRO_CI_NODE_BUILD_ID" => "142", + "KNAPSACK_PRO_CI_NODE_BUILD_ID" => "143", "KNAPSACK_PRO_TEST_SUITE_TOKEN_RSPEC" => "example_token", "KNAPSACK_PRO_FIXED_QUEUE_SPLIT" => "true" ) do diff --git a/spec/datadog/ci/contrib/knapsack_rspec_go/instrumentation_spec.rb b/spec/datadog/ci/contrib/knapsack_rspec_go/instrumentation_spec.rb new file mode 100644 index 00000000..db44b022 --- /dev/null +++ b/spec/datadog/ci/contrib/knapsack_rspec_go/instrumentation_spec.rb @@ -0,0 +1,79 @@ +require "knapsack_pro" +require "fileutils" + +RSpec.describe "Knapsack Pro runner when Datadog::CI is configured during the knapsack run like in rspec_go rake task" do + # Yields to a block in a new RSpec global context. All RSpec + # test configuration and execution should be wrapped in this method. + def with_new_rspec_environment + old_configuration = ::RSpec.configuration + old_world = ::RSpec.world + ::RSpec.configuration = ::RSpec::Core::Configuration.new + ::RSpec.world = ::RSpec::Core::World.new + + yield + ensure + ::RSpec.configuration = old_configuration + ::RSpec.world = old_world + end + + def devnull + File.new("/dev/null", "w") + end + + before do + allow_any_instance_of(Datadog::Core::Remote::Negotiation).to( + receive(:endpoint?).with("/evp_proxy/v4/").and_return(true) + ) + + allow(Datadog::CI::Utils::TestRun).to receive(:command).and_return("knapsack:queue:rspec") + + allow_any_instance_of(KnapsackPro::Runners::Queue::RSpecRunner).to receive(:test_file_paths).and_return( + ["./spec/datadog/ci/contrib/knapsack_rspec_go/suite_under_test/some_test_rspec.rb"], + [] + ) + end + + it "instruments this rspec session" do + with_new_rspec_environment do + ClimateControl.modify( + "KNAPSACK_PRO_CI_NODE_BUILD_ID" => "144", + "KNAPSACK_PRO_TEST_SUITE_TOKEN_RSPEC" => "example_token", + "KNAPSACK_PRO_FIXED_QUEUE_SPLIT" => "true", + "KNAPSACK_PRO_QUEUE_ID" => nil + ) do + KnapsackPro::Adapters::RSpecAdapter.bind + KnapsackPro::Runners::Queue::RSpecRunner.run("--require knapsack_helper", devnull, devnull) + rescue ArgumentError + # suppress invalid API key error + end + end + + # test session and module traced + expect(test_session_span).not_to be_nil + expect(test_module_span).not_to be_nil + + # test session and module are failed + expect([test_session_span, test_module_span]).to all have_fail_status + + # single test suite span + expect(test_suite_spans).to have(1).item + expect(test_suite_spans.first).to have_test_tag(:status, Datadog::CI::Ext::Test::Status::FAIL) + expect(test_suite_spans.first).to have_test_tag( + :suite, + "SomeTest at ./spec/datadog/ci/contrib/knapsack_rspec_go/suite_under_test/some_test_rspec.rb" + ) + + # there is test span for every test case + expect(test_spans).to have(2).items + # test spans belong to a single test suite + expect(test_spans).to have_unique_tag_values_count(:test_suite_id, 1) + expect(test_spans).to have_tag_values_no_order( + :status, + [Datadog::CI::Ext::Test::Status::FAIL, Datadog::CI::Ext::Test::Status::PASS] + ) + + # every test span is connected to test module and test session + expect(test_spans).to all have_test_tag(:test_module_id) + expect(test_spans).to all have_test_tag(:test_session_id) + end +end diff --git a/spec/datadog/ci/contrib/knapsack_rspec_go/suite_under_test/some_test_rspec.rb b/spec/datadog/ci/contrib/knapsack_rspec_go/suite_under_test/some_test_rspec.rb new file mode 100644 index 00000000..7b775cb9 --- /dev/null +++ b/spec/datadog/ci/contrib/knapsack_rspec_go/suite_under_test/some_test_rspec.rb @@ -0,0 +1,13 @@ +require "rspec" + +RSpec.describe "SomeTest" do + context "nested" do + it "foo" do + # DO NOTHING + end + + it "fails" do + expect(1).to eq(2) + end + end +end diff --git a/spec/knapsack_helper.rb b/spec/knapsack_helper.rb new file mode 100644 index 00000000..dadf2e05 --- /dev/null +++ b/spec/knapsack_helper.rb @@ -0,0 +1,8 @@ +# this file is required for knapsack unit tests + +Datadog.configure do |c| + c.service = "knapsack_rspec_example" + c.ci.enabled = true + c.ci.git_metadata_upload_enabled = false + c.ci.instrument :rspec +end diff --git a/spec/support/contexts/ci_mode.rb b/spec/support/contexts/ci_mode.rb index 2b7c59d0..f266c5c9 100644 --- a/spec/support/contexts/ci_mode.rb +++ b/spec/support/contexts/ci_mode.rb @@ -88,5 +88,9 @@ Datadog::CI.send(:itr_runner)&.shutdown! Datadog::CI.send(:recorder)&.shutdown! + + Datadog.configure do |c| + c.ci.enabled = false + end end end From 25bf96cb79a6cf84ab1289748e244f4c8ae25d60 Mon Sep 17 00:00:00 2001 From: Andrey Date: Wed, 22 May 2024 15:55:06 +0200 Subject: [PATCH 4/5] remove test that does not really test anything --- .../knapsack_rspec/instrumentation_spec.rb | 22 ------------------- 1 file changed, 22 deletions(-) diff --git a/spec/datadog/ci/contrib/knapsack_rspec/instrumentation_spec.rb b/spec/datadog/ci/contrib/knapsack_rspec/instrumentation_spec.rb index a631e1f8..549b6d0f 100644 --- a/spec/datadog/ci/contrib/knapsack_rspec/instrumentation_spec.rb +++ b/spec/datadog/ci/contrib/knapsack_rspec/instrumentation_spec.rb @@ -73,26 +73,4 @@ def devnull expect(test_spans).to all have_test_tag(:test_module_id) expect(test_spans).to all have_test_tag(:test_session_id) end - - context "when collecting examples with knapsack_pro:rspec_test_example_detector" do - it "does not instrument this rspec session" do - with_new_rspec_environment do - ClimateControl.modify( - "KNAPSACK_PRO_CI_NODE_BUILD_ID" => "143", - "KNAPSACK_PRO_TEST_SUITE_TOKEN_RSPEC" => "example_token", - "KNAPSACK_PRO_FIXED_QUEUE_SPLIT" => "true" - ) do - expect(KnapsackPro::SlowTestFileDeterminer).to receive(:read_from_json_report).and_return([]) - - detector = KnapsackPro::TestCaseDetectors::RSpecTestExampleDetector.new - detector.generate_json_report - rescue ArgumentError - # suppress invalid API key error - end - end - - expect(test_session_span).to be_nil - expect(test_spans).to be_empty - end - end end From 85152a26a1d4dc9598e383e23b2070a034506796 Mon Sep 17 00:00:00 2001 From: Andrey Date: Wed, 22 May 2024 16:19:13 +0200 Subject: [PATCH 5/5] do not use fully qualified name for KnapsackPro::Runner --- lib/datadog/ci/contrib/rspec/knapsack_pro/patcher.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/datadog/ci/contrib/rspec/knapsack_pro/patcher.rb b/lib/datadog/ci/contrib/rspec/knapsack_pro/patcher.rb index b66ac1c6..3346f6da 100644 --- a/lib/datadog/ci/contrib/rspec/knapsack_pro/patcher.rb +++ b/lib/datadog/ci/contrib/rspec/knapsack_pro/patcher.rb @@ -11,7 +11,7 @@ def self.patch ::RSpec::Core::Runner.ancestors.include?(::KnapsackPro::Extensions::RSpecExtension::Runner) # knapsack already patched rspec runner require_relative "runner" - ::RSpec::Core::Runner.include(Datadog::CI::Contrib::RSpec::KnapsackPro::Runner) + ::RSpec::Core::Runner.include(KnapsackPro::Runner) else # knapsack didn't patch rspec runner yet require_relative "extension"