From 7b6785b3b1d03ef8512e098285744e9956ec0891 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Fri, 15 Apr 2016 18:31:01 +0200 Subject: Use Module#prepend for method instrumentation By using Module#prepend we can define a Module containing all proxy methods. This removes the need for setting up crazy method alias chains and in turn prevents us from having to deal with all that madness (e.g. methods calling each other recursively). Fixes gitlab-org/gitlab-ce#15281 --- spec/lib/gitlab/metrics/instrumentation_spec.rb | 51 ++++++++++++++++++------- 1 file changed, 37 insertions(+), 14 deletions(-) (limited to 'spec/lib') diff --git a/spec/lib/gitlab/metrics/instrumentation_spec.rb b/spec/lib/gitlab/metrics/instrumentation_spec.rb index ad4290c43bb..5c885a7a982 100644 --- a/spec/lib/gitlab/metrics/instrumentation_spec.rb +++ b/spec/lib/gitlab/metrics/instrumentation_spec.rb @@ -33,8 +33,16 @@ describe Gitlab::Metrics::Instrumentation do described_class.instrument_method(@dummy, :foo) end - it 'renames the original method' do - expect(@dummy).to respond_to(:_original_foo) + it 'instruments the Class' do + target = @dummy.singleton_class + + expect(described_class.instrumented?(target)).to eq(true) + end + + it 'defines a proxy method' do + mod = described_class.proxy_module(@dummy.singleton_class) + + expect(mod.method_defined?(:foo)).to eq(true) end it 'calls the instrumented method with the correct arguments' do @@ -76,6 +84,14 @@ describe Gitlab::Metrics::Instrumentation do expect(dummy.method(:test).arity).to eq(0) end + + describe 'when a module is instrumented multiple times' do + it 'calls the instrumented method with the correct arguments' do + described_class.instrument_method(@dummy, :foo) + + expect(@dummy.foo).to eq('foo') + end + end end describe 'with metrics disabled' do @@ -86,7 +102,9 @@ describe Gitlab::Metrics::Instrumentation do it 'does not instrument the method' do described_class.instrument_method(@dummy, :foo) - expect(@dummy).to_not respond_to(:_original_foo) + target = @dummy.singleton_class + + expect(described_class.instrumented?(target)).to eq(false) end end end @@ -100,8 +118,14 @@ describe Gitlab::Metrics::Instrumentation do instrument_instance_method(@dummy, :bar) end - it 'renames the original method' do - expect(@dummy.method_defined?(:_original_bar)).to eq(true) + it 'instruments instances of the Class' do + expect(described_class.instrumented?(@dummy)).to eq(true) + end + + it 'defines a proxy method' do + mod = described_class.proxy_module(@dummy) + + expect(mod.method_defined?(:bar)).to eq(true) end it 'calls the instrumented method with the correct arguments' do @@ -144,7 +168,7 @@ describe Gitlab::Metrics::Instrumentation do described_class. instrument_instance_method(@dummy, :bar) - expect(@dummy.method_defined?(:_original_bar)).to eq(false) + expect(described_class.instrumented?(@dummy)).to eq(false) end end end @@ -167,18 +191,17 @@ describe Gitlab::Metrics::Instrumentation do it 'recursively instruments a class hierarchy' do described_class.instrument_class_hierarchy(@dummy) - expect(@child1).to respond_to(:_original_child1_foo) - expect(@child2).to respond_to(:_original_child2_foo) + expect(described_class.instrumented?(@child1.singleton_class)).to eq(true) + expect(described_class.instrumented?(@child2.singleton_class)).to eq(true) - expect(@child1.method_defined?(:_original_child1_bar)).to eq(true) - expect(@child2.method_defined?(:_original_child2_bar)).to eq(true) + expect(described_class.instrumented?(@child1)).to eq(true) + expect(described_class.instrumented?(@child2)).to eq(true) end it 'does not instrument the root module' do described_class.instrument_class_hierarchy(@dummy) - expect(@dummy).to_not respond_to(:_original_foo) - expect(@dummy.method_defined?(:_original_bar)).to eq(false) + expect(described_class.instrumented?(@dummy)).to eq(false) end end @@ -190,7 +213,7 @@ describe Gitlab::Metrics::Instrumentation do it 'instruments all public class methods' do described_class.instrument_methods(@dummy) - expect(@dummy).to respond_to(:_original_foo) + expect(described_class.instrumented?(@dummy.singleton_class)).to eq(true) end it 'only instruments methods directly defined in the module' do @@ -223,7 +246,7 @@ describe Gitlab::Metrics::Instrumentation do it 'instruments all public instance methods' do described_class.instrument_instance_methods(@dummy) - expect(@dummy.method_defined?(:_original_bar)).to eq(true) + expect(described_class.instrumented?(@dummy)).to eq(true) end it 'only instruments methods directly defined in the module' do -- cgit v1.2.1 From a1363d39c6fe79d830dbce468c02880d2a5d7996 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 8 Apr 2016 11:02:26 +0200 Subject: Add `variables` keyword to job in CI config YAML --- spec/lib/ci/gitlab_ci_yaml_processor_spec.rb | 44 ++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 12 deletions(-) (limited to 'spec/lib') diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb index dcb8a3451bd..a3a0d06e149 100644 --- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb +++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb @@ -346,19 +346,39 @@ module Ci end describe "Variables" do - it "returns variables when defined" do - variables = { - var1: "value1", - var2: "value2", - } - config = YAML.dump({ - variables: variables, - before_script: ["pwd"], - rspec: { script: "rspec" } - }) + context 'when global variables are defined' do + it 'returns variables' do + variables = { + var1: "value1", + var2: "value2", + } + config = YAML.dump({ + variables: variables, + before_script: ["pwd"], + rspec: { script: "rspec" } + }) - config_processor = GitlabCiYamlProcessor.new(config, path) - expect(config_processor.variables).to eq(variables) + config_processor = GitlabCiYamlProcessor.new(config, path) + expect(config_processor.variables).to eq(variables) + end + end + + context 'when job variables are defined' do + let(:job_variables) { { KEY1: 'value1', SOME_KEY_2: 'value2'} } + let(:yaml_config) do + YAML.dump( + { before_script: ['pwd'], + rspec: { + variables: job_variables, + script: 'rspec' } + }) + end + + it 'appends job variable to job attributes' do + config = GitlabCiYamlProcessor.new(yaml_config, path) + + expect(config.builds.first[:options][:variables]).to eq job_variables + end end end -- cgit v1.2.1 From b578fbfb8572860490cdfd0163bfbf5f999bb1e6 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 11 Apr 2016 13:09:46 +0200 Subject: Make it possible to override build variables --- spec/lib/ci/gitlab_ci_yaml_processor_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'spec/lib') diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb index a3a0d06e149..c2908f855e3 100644 --- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb +++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb @@ -364,7 +364,7 @@ module Ci end context 'when job variables are defined' do - let(:job_variables) { { KEY1: 'value1', SOME_KEY_2: 'value2'} } + let(:job_variables) { { KEY1: 'value1', SOME_KEY_2: 'value2' } } let(:yaml_config) do YAML.dump( { before_script: ['pwd'], -- cgit v1.2.1 From b7946b50fc1b23b1974f7d0306c06f6d92cc8466 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 15 Apr 2016 12:18:46 +0200 Subject: Read job variables directly from gitlab CI config --- spec/lib/ci/gitlab_ci_yaml_processor_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'spec/lib') diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb index c2908f855e3..04b1d8baeb2 100644 --- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb +++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb @@ -374,10 +374,10 @@ module Ci }) end - it 'appends job variable to job attributes' do + it 'returns job variables' do config = GitlabCiYamlProcessor.new(yaml_config, path) - expect(config.builds.first[:options][:variables]).to eq job_variables + expect(config.job_variables(:rspec)).to eq job_variables end end end -- cgit v1.2.1 From 3dec6e262984faa18b235db4bbd669ccdc80fc3f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 15 Apr 2016 12:28:27 +0200 Subject: Rename method that returns global envs in CI conf --- spec/lib/ci/gitlab_ci_yaml_processor_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'spec/lib') diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb index 04b1d8baeb2..ba5f7b0e09a 100644 --- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb +++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb @@ -359,7 +359,7 @@ module Ci }) config_processor = GitlabCiYamlProcessor.new(config, path) - expect(config_processor.variables).to eq(variables) + expect(config_processor.global_variables).to eq(variables) end end -- cgit v1.2.1 From 1f3248644ef98879a8a4c31d5be0de9a774cc32a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 15 Apr 2016 14:57:22 +0200 Subject: Make CI config return empty array if no job variables --- spec/lib/ci/gitlab_ci_yaml_processor_spec.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'spec/lib') diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb index ba5f7b0e09a..45f08c95cc3 100644 --- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb +++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb @@ -380,6 +380,18 @@ module Ci expect(config.job_variables(:rspec)).to eq job_variables end end + + context 'when job variables are not defined' do + it 'returns empty array' do + config = YAML.dump({ + before_script: ["pwd"], + rspec: { script: "rspec" } + }) + + config_processor = GitlabCiYamlProcessor.new(config, path) + expect(config_processor.job_variables(:rspec)).to eq [] + end + end end describe "When" do -- cgit v1.2.1 From cf3e3effb01215c2556ba509d3cb4debe1d901e8 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sat, 16 Apr 2016 20:52:01 +0200 Subject: Minor refactoring in code related to job variables --- spec/lib/ci/gitlab_ci_yaml_processor_spec.rb | 43 ++++++++++++++++------------ 1 file changed, 24 insertions(+), 19 deletions(-) (limited to 'spec/lib') diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb index 45f08c95cc3..9a014c4b6c6 100644 --- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb +++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb @@ -345,50 +345,55 @@ module Ci end end - describe "Variables" do + describe 'Variables' do context 'when global variables are defined' do - it 'returns variables' do + it 'returns global variables' do variables = { - var1: "value1", - var2: "value2", + VAR1: 'value1', + VAR2: 'value2', } + config = YAML.dump({ - variables: variables, - before_script: ["pwd"], - rspec: { script: "rspec" } - }) + variables: variables, + before_script: ['pwd'], + rspec: { script: 'rspec' } + }) config_processor = GitlabCiYamlProcessor.new(config, path) + expect(config_processor.global_variables).to eq(variables) end end context 'when job variables are defined' do - let(:job_variables) { { KEY1: 'value1', SOME_KEY_2: 'value2' } } - let(:yaml_config) do - YAML.dump( + it 'returns job variables' do + variables = { + KEY1: 'value1', + SOME_KEY_2: 'value2' + } + + config = YAML.dump( { before_script: ['pwd'], rspec: { - variables: job_variables, + variables: variables, script: 'rspec' } }) - end - it 'returns job variables' do - config = GitlabCiYamlProcessor.new(yaml_config, path) + config_processor = GitlabCiYamlProcessor.new(config, path) - expect(config.job_variables(:rspec)).to eq job_variables + expect(config_processor.job_variables(:rspec)).to eq variables end end context 'when job variables are not defined' do it 'returns empty array' do config = YAML.dump({ - before_script: ["pwd"], - rspec: { script: "rspec" } - }) + before_script: ['pwd'], + rspec: { script: 'rspec' } + }) config_processor = GitlabCiYamlProcessor.new(config, path) + expect(config_processor.job_variables(:rspec)).to eq [] end end -- cgit v1.2.1 From 2b1c08be8fa5c18f1fe151feb2a0938734086481 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sun, 17 Apr 2016 20:23:42 +0200 Subject: Validate job-level variables in YAML config file --- spec/lib/ci/gitlab_ci_yaml_processor_spec.rb | 45 ++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 13 deletions(-) (limited to 'spec/lib') diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb index 9a014c4b6c6..8813a724774 100644 --- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb +++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb @@ -366,22 +366,41 @@ module Ci end context 'when job variables are defined' do - it 'returns job variables' do - variables = { - KEY1: 'value1', - SOME_KEY_2: 'value2' - } + context 'when syntax is correct' do + it 'returns job variables' do + variables = { + KEY1: 'value1', + SOME_KEY_2: 'value2' + } - config = YAML.dump( - { before_script: ['pwd'], - rspec: { - variables: variables, - script: 'rspec' } - }) + config = YAML.dump( + { before_script: ['pwd'], + rspec: { + variables: variables, + script: 'rspec' } + }) - config_processor = GitlabCiYamlProcessor.new(config, path) + config_processor = GitlabCiYamlProcessor.new(config, path) + + expect(config_processor.job_variables(:rspec)).to eq variables + end + end - expect(config_processor.job_variables(:rspec)).to eq variables + context 'when syntax is incorrect' do + it 'raises error' do + variables = [:KEY1, 'value1', :KEY2, 'value2'] + + config = YAML.dump( + { before_script: ['pwd'], + rspec: { + variables: variables, + script: 'rspec' } + }) + + expect { GitlabCiYamlProcessor.new(config, path) } + .to raise_error(GitlabCiYamlProcessor::ValidationError, + /job: variables should be a map/) + end end end -- cgit v1.2.1 From 1339fda1cd3325c0186b5f1b53444e7319ad3cb6 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 18 Apr 2016 12:41:13 +0200 Subject: Minor refactorings in CI config --- spec/lib/ci/gitlab_ci_yaml_processor_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'spec/lib') diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb index 8813a724774..5f4b63bcafb 100644 --- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb +++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb @@ -786,14 +786,14 @@ EOT config = YAML.dump({ variables: "test", rspec: { script: "test" } }) expect do GitlabCiYamlProcessor.new(config, path) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "variables should be a map of key-valued strings") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "variables should be a map of key-value strings") end - it "returns errors if variables is not a map of key-valued strings" do + it "returns errors if variables is not a map of key-value strings" do config = YAML.dump({ variables: { test: false }, rspec: { script: "test" } }) expect do GitlabCiYamlProcessor.new(config, path) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "variables should be a map of key-valued strings") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "variables should be a map of key-value strings") end it "returns errors if job when is not on_success, on_failure or always" do -- cgit v1.2.1