From 9d8e7e86df2b5d3bfeb8ae0cca511e7e703a889c Mon Sep 17 00:00:00 2001 From: Andrey Date: Tue, 1 Oct 2024 11:01:12 +0200 Subject: [PATCH 1/7] first pass on optimisation of relative_to_root helper --- lib/datadog/ci/git/local_repository.rb | 28 +++++++++++++++++-- .../test_optimisation/coverage/event_spec.rb | 2 +- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/lib/datadog/ci/git/local_repository.rb b/lib/datadog/ci/git/local_repository.rb index 8c2f1a6e..07dba9a6 100644 --- a/lib/datadog/ci/git/local_repository.rb +++ b/lib/datadog/ci/git/local_repository.rb @@ -36,10 +36,32 @@ def self.relative_to_root(path) root_path = root return path if root_path.nil? - path = Pathname.new(File.expand_path(path)) - root_path = Pathname.new(root_path) + if File.absolute_path?(path) + res = path.gsub(root, "") + if res[0] == File::SEPARATOR + res = res[1..] + end + else + if @prefix && @prefix != "" + return "#{@prefix}#{path}" + end + + pathname = Pathname.new(File.expand_path(path)) + root_path = Pathname.new(root_path) + + # relative_path_from is an expensive function + res = pathname.relative_path_from(root_path).to_s + + unless defined?(@prefix) + @prefix = if res.end_with?(path) + res&.gsub(path, "") + else + nil + end + end + end - path.relative_path_from(root_path).to_s + res || "" end def self.repository_name diff --git a/spec/datadog/ci/test_optimisation/coverage/event_spec.rb b/spec/datadog/ci/test_optimisation/coverage/event_spec.rb index 094cbb58..ec1dc6e6 100644 --- a/spec/datadog/ci/test_optimisation/coverage/event_spec.rb +++ b/spec/datadog/ci/test_optimisation/coverage/event_spec.rb @@ -86,7 +86,7 @@ context "when file paths are absolute" do let(:coverage) do { - absolute_path("./project/file.rb") => true + absolute_path("project/file.rb") => true } end From b12f028b30b5acafbbf36591e52c1330012a2161 Mon Sep 17 00:00:00 2001 From: Andrey Date: Tue, 1 Oct 2024 12:41:40 +0200 Subject: [PATCH 2/7] optimise .relative_to_root more --- lib/datadog/ci/git/local_repository.rb | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/lib/datadog/ci/git/local_repository.rb b/lib/datadog/ci/git/local_repository.rb index 07dba9a6..fe67a493 100644 --- a/lib/datadog/ci/git/local_repository.rb +++ b/lib/datadog/ci/git/local_repository.rb @@ -37,13 +37,18 @@ def self.relative_to_root(path) return path if root_path.nil? if File.absolute_path?(path) - res = path.gsub(root, "") - if res[0] == File::SEPARATOR - res = res[1..] - end + prefix_index = root_path.size + + # impossible case + return "" if path.size < prefix_index + + prefix_index += 1 if path[prefix_index] == File::SEPARATOR + res = path[prefix_index..] else - if @prefix && @prefix != "" - return "#{@prefix}#{path}" + if @prefix_to_root == "" + return path + elsif @prefix_to_root + return File.join(@prefix_to_root, path) end pathname = Pathname.new(File.expand_path(path)) @@ -52,12 +57,8 @@ def self.relative_to_root(path) # relative_path_from is an expensive function res = pathname.relative_path_from(root_path).to_s - unless defined?(@prefix) - @prefix = if res.end_with?(path) - res&.gsub(path, "") - else - nil - end + unless defined?(@prefix_to_root) + @prefix_to_root = res&.gsub(path, "") if res.end_with?(path) end end From 9a24041394daec33458950881e51732e0a64d6f4 Mon Sep 17 00:00:00 2001 From: Andrey Date: Tue, 1 Oct 2024 13:28:21 +0200 Subject: [PATCH 3/7] provide 0 response size when request failed --- lib/datadog/ci/transport/http.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/datadog/ci/transport/http.rb b/lib/datadog/ci/transport/http.rb index 74301f04..64af304a 100644 --- a/lib/datadog/ci/transport/http.rb +++ b/lib/datadog/ci/transport/http.rb @@ -149,6 +149,10 @@ def code nil end + def response_size + 0 + end + def inspect "ErrorResponse error:#{error}" end From 98f03e51dfc24584ca27b6f4091c94702f52789f Mon Sep 17 00:00:00 2001 From: Andrey Date: Tue, 1 Oct 2024 14:14:04 +0200 Subject: [PATCH 4/7] add some comments and additional test case --- lib/datadog/ci/git/local_repository.rb | 7 +++- .../test_optimisation/coverage/event_spec.rb | 35 +++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/lib/datadog/ci/git/local_repository.rb b/lib/datadog/ci/git/local_repository.rb index fe67a493..cbd71e7f 100644 --- a/lib/datadog/ci/git/local_repository.rb +++ b/lib/datadog/ci/git/local_repository.rb @@ -30,6 +30,8 @@ def self.root @root = git_root || Dir.pwd end + # ATTENTION: this function is running in a hot path + # and should be optimized for performance def self.relative_to_root(path) return "" if path.nil? @@ -37,14 +39,17 @@ def self.relative_to_root(path) return path if root_path.nil? if File.absolute_path?(path) + # prefix_index is where the root path ends in the given path prefix_index = root_path.size - # impossible case + # impossible case - absolute paths are returned from code coverage tool that always checks + # that root is a prefix of the path return "" if path.size < prefix_index prefix_index += 1 if path[prefix_index] == File::SEPARATOR res = path[prefix_index..] else + # prefix_to_root is a difference between the root path and the given path if @prefix_to_root == "" return path elsif @prefix_to_root diff --git a/spec/datadog/ci/test_optimisation/coverage/event_spec.rb b/spec/datadog/ci/test_optimisation/coverage/event_spec.rb index ec1dc6e6..238dd66b 100644 --- a/spec/datadog/ci/test_optimisation/coverage/event_spec.rb +++ b/spec/datadog/ci/test_optimisation/coverage/event_spec.rb @@ -104,6 +104,41 @@ end end + context "when file paths are relative" do + before do + Datadog::CI::Git::LocalRepository.remove_instance_variable(:@prefix_to_root) + + allow(Datadog::CI::Git::LocalRepository).to receive(:root).and_return( + Dir.pwd.gsub("/datadog-ci-rb", "") + ) + end + + after do + Datadog::CI::Git::LocalRepository.remove_instance_variable(:@prefix_to_root) + end + + let(:coverage) do + { + "project/file.rb" => true, + "project/file2.rb" => true + } + end + + it "converts all file paths to relative to the git root" do + expect(msgpack_json).to eq( + { + "test_session_id" => 3, + "test_suite_id" => 2, + "span_id" => 1, + "files" => [ + {"filename" => "datadog-ci-rb/project/file.rb"}, + {"filename" => "datadog-ci-rb/project/file2.rb"} + ] + } + ) + end + end + context "coverage in lines format" do let(:coverage) { {"file.rb" => {1 => true, 2 => true, 3 => true}} } From 22d3cc45e56dcb58015841a7aeaf26cd8684a74a Mon Sep 17 00:00:00 2001 From: Andrey Date: Tue, 1 Oct 2024 14:28:19 +0200 Subject: [PATCH 5/7] fix failing tests --- spec/datadog/ci/test_optimisation/coverage/event_spec.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/spec/datadog/ci/test_optimisation/coverage/event_spec.rb b/spec/datadog/ci/test_optimisation/coverage/event_spec.rb index 238dd66b..cf54de43 100644 --- a/spec/datadog/ci/test_optimisation/coverage/event_spec.rb +++ b/spec/datadog/ci/test_optimisation/coverage/event_spec.rb @@ -105,11 +105,13 @@ end context "when file paths are relative" do + let(:current_folder) { File.basename(Dir.pwd) } + before do Datadog::CI::Git::LocalRepository.remove_instance_variable(:@prefix_to_root) allow(Datadog::CI::Git::LocalRepository).to receive(:root).and_return( - Dir.pwd.gsub("/datadog-ci-rb", "") + Dir.pwd.gsub("/#{current_folder}", "") ) end @@ -131,8 +133,8 @@ "test_suite_id" => 2, "span_id" => 1, "files" => [ - {"filename" => "datadog-ci-rb/project/file.rb"}, - {"filename" => "datadog-ci-rb/project/file2.rb"} + {"filename" => "#{current_folder}/project/file.rb"}, + {"filename" => "#{current_folder}/project/file2.rb"} ] } ) From f6621e806f8cbb1d154cc30d46da7598ded4c1e8 Mon Sep 17 00:00:00 2001 From: Andrey Date: Tue, 1 Oct 2024 14:31:28 +0200 Subject: [PATCH 6/7] try to fix tests agasin --- spec/datadog/ci/test_optimisation/coverage/event_spec.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/datadog/ci/test_optimisation/coverage/event_spec.rb b/spec/datadog/ci/test_optimisation/coverage/event_spec.rb index cf54de43..21052209 100644 --- a/spec/datadog/ci/test_optimisation/coverage/event_spec.rb +++ b/spec/datadog/ci/test_optimisation/coverage/event_spec.rb @@ -108,7 +108,9 @@ let(:current_folder) { File.basename(Dir.pwd) } before do - Datadog::CI::Git::LocalRepository.remove_instance_variable(:@prefix_to_root) + if Datadog::CI::Git::LocalRepository.instance_variable_defined?(:@prefix_to_root) + Datadog::CI::Git::LocalRepository.remove_instance_variable(:@prefix_to_root) + end allow(Datadog::CI::Git::LocalRepository).to receive(:root).and_return( Dir.pwd.gsub("/#{current_folder}", "") From 777af476745b9b0c0ebf7216027e037362a363cb Mon Sep 17 00:00:00 2001 From: Andrey Date: Tue, 1 Oct 2024 14:34:31 +0200 Subject: [PATCH 7/7] circle ci runs tests in /app folder --- spec/datadog/ci/test_optimisation/coverage/event_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/datadog/ci/test_optimisation/coverage/event_spec.rb b/spec/datadog/ci/test_optimisation/coverage/event_spec.rb index 21052209..9eea1445 100644 --- a/spec/datadog/ci/test_optimisation/coverage/event_spec.rb +++ b/spec/datadog/ci/test_optimisation/coverage/event_spec.rb @@ -112,9 +112,9 @@ Datadog::CI::Git::LocalRepository.remove_instance_variable(:@prefix_to_root) end - allow(Datadog::CI::Git::LocalRepository).to receive(:root).and_return( - Dir.pwd.gsub("/#{current_folder}", "") - ) + new_root = Dir.pwd.gsub("/#{current_folder}", "") + new_root = "/" if new_root.empty? + allow(Datadog::CI::Git::LocalRepository).to receive(:root).and_return(new_root) end after do