From 0f6a609763122abbb724a3e9d86c5e53d6c46aea Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Mon, 19 Aug 2019 21:42:22 +0800 Subject: [PATCH 1/3] (maint) Refactor aggregate testing Previously the tests use private instance methods to verify the object, however the each_list method now does that. This commit changes the tests to use this method instead. --- .../featureflag_puppetstrings_spec.rb | 38 +++++++------------ 1 file changed, 13 insertions(+), 25 deletions(-) diff --git a/spec/languageserver-sidecar/integration/puppet-languageserver-sidecar/featureflag_puppetstrings/featureflag_puppetstrings_spec.rb b/spec/languageserver-sidecar/integration/puppet-languageserver-sidecar/featureflag_puppetstrings/featureflag_puppetstrings_spec.rb index 093b42ea..bb828cac 100644 --- a/spec/languageserver-sidecar/integration/puppet-languageserver-sidecar/featureflag_puppetstrings/featureflag_puppetstrings_spec.rb +++ b/spec/languageserver-sidecar/integration/puppet-languageserver-sidecar/featureflag_puppetstrings/featureflag_puppetstrings_spec.rb @@ -111,14 +111,10 @@ def expect_same_array_content(a, b) deserial2 = PuppetLanguageServer::Sidecar::Protocol::AggregateMetadata.new() expect { deserial2.from_json!(result2) }.to_not raise_error - deserial.class - .instance_methods(false) - .reject { |name| %i[to_json from_json! each_list append!].include?(name) } - .each do |method_name| - # There should be at least one item - expect(deserial.send(method_name).count).to be > 0 - # Before and after should be the same - expect_same_array_content(deserial.send(method_name), deserial2.send(method_name)) + deserial.each_list do |key, value| + # There should be at least one item per list in the aggregate + expect(value.count).to be > 0 + expect_same_array_content(value, deserial2.send(key)) end end end @@ -270,7 +266,7 @@ def expect_same_array_content(a, b) describe 'when running workspace_aggregate action' do let (:cmd_options) { ['--action', 'workspace_aggregate', '--local-workspace', workspace] } - it 'should return a cachable deserializable aggregate object with all default metadata' do + it 'should return a cachable deserializable aggregate object with all workspace metadata' do expect_empty_cache result = run_sidecar(cmd_options) @@ -286,14 +282,10 @@ def expect_same_array_content(a, b) deserial2 = PuppetLanguageServer::Sidecar::Protocol::AggregateMetadata.new() expect { deserial2.from_json!(result2) }.to_not raise_error - deserial.class - .instance_methods(false) - .reject { |name| %i[to_json from_json! each_list append!].include?(name) } - .each do |method_name| - # There should be at least one item - expect(deserial.send(method_name).count).to be > 0 - # Before and after should be the same - expect_same_array_content(deserial.send(method_name), deserial2.send(method_name)) + deserial.each_list do |key, value| + # There should be at least one item per list in the aggregate + expect(value.count).to be > 0 + expect_same_array_content(value, deserial2.send(key)) end end end @@ -429,14 +421,10 @@ def expect_same_array_content(a, b) deserial2 = PuppetLanguageServer::Sidecar::Protocol::AggregateMetadata.new() expect { deserial2.from_json!(result2) }.to_not raise_error - deserial.class - .instance_methods(false) - .reject { |name| %i[to_json from_json! each_list append!].include?(name) } - .each do |method_name| - # There should be at least one item - expect(deserial.send(method_name).count).to be > 0 - # Before and after should be the same - expect_same_array_content(deserial.send(method_name), deserial2.send(method_name)) + deserial.each_list do |key, value| + # There should be at least one item per list in the aggregate + expect(value.count).to be > 0 + expect_same_array_content(value, deserial2.send(key)) end end end From 685ad8e0a4b7018a3792a02595b7960f8adcdf99 Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Mon, 19 Aug 2019 21:44:34 +0800 Subject: [PATCH 2/3] (GH-163) Allow the aggregate to be used in all cases Previously the metadata aggregate action could only be used with the puppetstrings feature flag. However it is also very useful for normal operation. This commit changes the action to be valid, and adds tests for this scenario. --- lib/puppet-languageserver/sidecar_protocol.rb | 5 +++ lib/puppet_languageserver_sidecar.rb | 16 ++++++--- .../puppet-languageserver-sidecar_spec.rb | 33 +++++++++++++++---- 3 files changed, 44 insertions(+), 10 deletions(-) diff --git a/lib/puppet-languageserver/sidecar_protocol.rb b/lib/puppet-languageserver/sidecar_protocol.rb index 68123a8f..999e1f56 100644 --- a/lib/puppet-languageserver/sidecar_protocol.rb +++ b/lib/puppet-languageserver/sidecar_protocol.rb @@ -385,6 +385,11 @@ def types @aggregate[:types] end + def concat!(array) + return if array.nil? || array.empty? + array.each { |item| append!(item) } + end + def append!(obj) list_for_object_class(obj.class) << obj end diff --git a/lib/puppet_languageserver_sidecar.rb b/lib/puppet_languageserver_sidecar.rb index 0e686654..464c323d 100644 --- a/lib/puppet_languageserver_sidecar.rb +++ b/lib/puppet_languageserver_sidecar.rb @@ -256,6 +256,7 @@ def self.inject_workspace_as_environment def self.execute(options) use_puppet_strings = featureflag?('puppetstrings') + log_message(:debug, "Executing #{options[:action]} action") case options[:action].downcase when 'noop' [] @@ -265,8 +266,7 @@ def self.execute(options) if use_puppet_strings PuppetLanguageServerSidecar::PuppetHelper.retrieve_via_puppet_strings(cache, :object_types => PuppetLanguageServerSidecar::PuppetHelper.available_documentation_types) else - log_message(:warn, 'The default_aggregate action is only supported with the puppetstrings feature flag') - {} + create_aggregate(cache) end when 'default_classes' @@ -326,8 +326,7 @@ def self.execute(options) :object_types => PuppetLanguageServerSidecar::PuppetHelper.available_documentation_types, :root_path => PuppetLanguageServerSidecar::Workspace.root_path) else - log_message(:warn, 'The workspace_aggregate action is only supported with the puppetstrings feature flag') - {} + create_aggregate(PuppetLanguageServerSidecar::Cache::Null.new, PuppetLanguageServerSidecar::Workspace.root_path) end when 'workspace_classes' @@ -374,6 +373,15 @@ def self.execute(options) end end + def self.create_aggregate(cache, root_path = nil) + result = PuppetLanguageServer::Sidecar::Protocol::AggregateMetadata.new + result.concat!(PuppetLanguageServerSidecar::PuppetHelper.retrieve_types(cache, :root_path => root_path)) + result.concat!(PuppetLanguageServerSidecar::PuppetHelper.retrieve_functions(cache, :root_path => root_path)) + result.concat!(PuppetLanguageServerSidecar::PuppetHelper.retrieve_classes(cache, :root_path => root_path)) + result + end + private_class_method :create_aggregate + def self.output(result, options) if options[:output].nil? || options[:output].empty? STDOUT.binmode diff --git a/spec/languageserver-sidecar/integration/puppet-languageserver-sidecar/puppet-languageserver-sidecar_spec.rb b/spec/languageserver-sidecar/integration/puppet-languageserver-sidecar/puppet-languageserver-sidecar_spec.rb index e25e8844..98a31771 100644 --- a/spec/languageserver-sidecar/integration/puppet-languageserver-sidecar/puppet-languageserver-sidecar_spec.rb +++ b/spec/languageserver-sidecar/integration/puppet-languageserver-sidecar/puppet-languageserver-sidecar_spec.rb @@ -48,10 +48,17 @@ def with_temporary_file(content) describe 'when running default_aggregate action' do let (:cmd_options) { ['--action', 'default_aggregate'] } - it 'should return an empty hash' do + it 'should return a deserializable aggregate object with all default metadata' do result = run_sidecar(cmd_options) + deserial = PuppetLanguageServer::Sidecar::Protocol::AggregateMetadata.new + expect { deserial.from_json!(result) }.to_not raise_error + + # The contents of the result are tested later - expect(result).to eq('{}') + # There should be at least one item per list in the aggregate + deserial.each_list do |_, v| + expect(v.count).to be > 0 + end end end @@ -139,10 +146,17 @@ def with_temporary_file(content) describe 'when running workspace_aggregate action' do let (:cmd_options) { ['--action', 'workspace_aggregate', '--local-workspace', workspace] } - it 'should return an empty hash' do + it 'should return a deserializable aggregate object with all workspace metadata' do result = run_sidecar(cmd_options) + deserial = PuppetLanguageServer::Sidecar::Protocol::AggregateMetadata.new + expect { deserial.from_json!(result) }.to_not raise_error + + # The contents of the result are tested later - expect(result).to eq('{}') + # There should be at least one item per list in the aggregate + deserial.each_list do |_, v| + expect(v.count).to be > 0 + end end end @@ -235,10 +249,17 @@ def with_temporary_file(content) describe 'when running workspace_aggregate action' do let (:cmd_options) { ['--action', 'workspace_aggregate', '--local-workspace', workspace] } - it 'should return an empty hash' do + it 'should return a deserializable aggregate object with all workspace metadata' do result = run_sidecar(cmd_options) + deserial = PuppetLanguageServer::Sidecar::Protocol::AggregateMetadata.new + expect { deserial.from_json!(result) }.to_not raise_error - expect(result).to eq('{}') + # The contents of the result are tested later + + # There should be at least one item per list in the aggregate + deserial.each_list do |_, v| + expect(v.count).to be > 0 + end end end From 376fc9e4d870f95a997fa7f24322f41f8a4dd9c3 Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Tue, 20 Aug 2019 16:18:20 +0800 Subject: [PATCH 3/3] (GH-163) Use the aggregate action for puppetstrings feature This commit changes the language server to use the default and workspace aggregate actions, but behind the puppetstrings feature. There are no test changes required. --- .../puppet_helper_puppetstrings.rb | 2 +- lib/puppet-languageserver/puppet_helper.rb | 12 ++++++ .../puppet_helper/cache.rb | 1 + lib/puppet-languageserver/sidecar_queue.rb | 32 ++++++++++++++ lib/puppet_languageserver.rb | 42 +++++++++++++++---- .../sidecar_queue_spec.rb | 22 ++++++++++ 6 files changed, 102 insertions(+), 9 deletions(-) diff --git a/lib/puppet-languageserver-sidecar/puppet_helper_puppetstrings.rb b/lib/puppet-languageserver-sidecar/puppet_helper_puppetstrings.rb index aad12ae8..e5adc51e 100644 --- a/lib/puppet-languageserver-sidecar/puppet_helper_puppetstrings.rb +++ b/lib/puppet-languageserver-sidecar/puppet_helper_puppetstrings.rb @@ -83,7 +83,7 @@ def self.retrieve_via_puppet_strings(cache, options = {}) end # Remove Puppet3 functions which have a Puppet4 function already loaded - if object_types.include?(:function) + if object_types.include?(:function) && !result.functions.nil? pup4_functions = result.functions.select { |i| i.function_version == 4 }.map { |i| i.key } result.functions.reject! { |i| i.function_version == 3 && pup4_functions.include?(i.key) } end diff --git a/lib/puppet-languageserver/puppet_helper.rb b/lib/puppet-languageserver/puppet_helper.rb index 67d987f6..7b256dfb 100644 --- a/lib/puppet-languageserver/puppet_helper.rb +++ b/lib/puppet-languageserver/puppet_helper.rb @@ -181,6 +181,11 @@ def self.cache # Workspace Loading def self.load_workspace_async + if PuppetLanguageServer.featureflag?('puppetstrings') + return true if PuppetLanguageServer::DocumentStore.store_root_path.nil? + sidecar_queue.enqueue('workspace_aggregate', ['--local-workspace', PuppetLanguageServer::DocumentStore.store_root_path]) + return true + end load_workspace_classes_async load_workspace_functions_async load_workspace_types_async @@ -226,5 +231,12 @@ def self.with_temporary_file(content) tempfile.delete if tempfile end private_class_method :with_temporary_file + + def self.load_default_aggregate_async + @default_classes_loaded = false if @default_classes_loaded.nil? + @default_functions_loaded = false if @default_functions_loaded.nil? + @default_types_loaded = false if @default_types_loaded.nil? + sidecar_queue.enqueue('default_aggregate', []) + end end end diff --git a/lib/puppet-languageserver/puppet_helper/cache.rb b/lib/puppet-languageserver/puppet_helper/cache.rb index 7c478b2f..a119e2ff 100644 --- a/lib/puppet-languageserver/puppet_helper/cache.rb +++ b/lib/puppet-languageserver/puppet_helper/cache.rb @@ -12,6 +12,7 @@ def initialize(_options = {}) def import_sidecar_list!(list, section, origin = nil) section_object = section_to_object(section) return if section_object.nil? + list = [] if list.nil? @cache_lock.synchronize do # Remove the existing items diff --git a/lib/puppet-languageserver/sidecar_queue.rb b/lib/puppet-languageserver/sidecar_queue.rb index 3888fa9f..295793d7 100644 --- a/lib/puppet-languageserver/sidecar_queue.rb +++ b/lib/puppet-languageserver/sidecar_queue.rb @@ -63,6 +63,24 @@ def execute_sync(action, additional_args, handle_errors = false) result = stdout.bytes.pack('U*') case action.downcase + when 'default_aggregate' + lists = PuppetLanguageServer::Sidecar::Protocol::AggregateMetadata.new.from_json!(result) + @cache.import_sidecar_list!(lists.classes, :class, :default) + @cache.import_sidecar_list!(lists.functions, :function, :default) + @cache.import_sidecar_list!(lists.types, :type, :default) + + PuppetLanguageServer::PuppetHelper.assert_default_classes_loaded + PuppetLanguageServer::PuppetHelper.assert_default_functions_loaded + PuppetLanguageServer::PuppetHelper.assert_default_types_loaded + + lists.each_list do |k, v| + if v.nil? + PuppetLanguageServer.log_message(:debug, "SidecarQueue Thread: default_aggregate returned no #{k}") + else + PuppetLanguageServer.log_message(:debug, "SidecarQueue Thread: default_aggregate returned #{v.count} #{k}") + end + end + when 'default_classes' list = PuppetLanguageServer::Sidecar::Protocol::PuppetClassList.new.from_json!(result) @cache.import_sidecar_list!(list, :class, :default) @@ -90,6 +108,20 @@ def execute_sync(action, additional_args, handle_errors = false) when 'resource_list' return PuppetLanguageServer::Sidecar::Protocol::ResourceList.new.from_json!(result) + when 'workspace_aggregate' + lists = PuppetLanguageServer::Sidecar::Protocol::AggregateMetadata.new.from_json!(result) + @cache.import_sidecar_list!(lists.classes, :class, :workspace) + @cache.import_sidecar_list!(lists.functions, :function, :workspace) + @cache.import_sidecar_list!(lists.types, :type, :workspace) + + lists.each_list do |k, v| + if v.nil? + PuppetLanguageServer.log_message(:debug, "SidecarQueue Thread: workspace_aggregate returned no #{k}") + else + PuppetLanguageServer.log_message(:debug, "SidecarQueue Thread: workspace_aggregate returned #{v.count} #{k}") + end + end + when 'workspace_classes' list = PuppetLanguageServer::Sidecar::Protocol::PuppetClassList.new.from_json!(result) @cache.import_sidecar_list!(list, :class, :workspace) diff --git a/lib/puppet_languageserver.rb b/lib/puppet_languageserver.rb index a11be38c..e908c19a 100644 --- a/lib/puppet_languageserver.rb +++ b/lib/puppet_languageserver.rb @@ -24,6 +24,15 @@ def self.active? @server_is_active end + def self.configure_featureflags(flags) + @flags = flags + end + + def self.featureflag?(flagname) + return false if @flags.nil? || @flags.empty? + @flags.include?(flagname) + end + def self.require_gems(options) original_verbose = $VERBOSE $VERBOSE = nil @@ -83,6 +92,15 @@ def self.require_gems(options) require File.expand_path(File.join(File.dirname(__FILE__), 'puppet-languageserver', lib)) end end + + # Validate the feature flags + unless options[:flags].nil? || options[:flags].empty? + flags = options[:flags] + log_message(:debug, "Detected feature flags [#{flags.join(', ')}]") + + configure_featureflags(flags) + end + @server_is_active = true ensure $VERBOSE = original_verbose @@ -223,17 +241,25 @@ def self.init_puppet_worker(options) log_message(:info, "Using Facter v#{Facter.version}") if options[:preload_puppet] - log_message(:info, 'Preloading Puppet Types (Async)...') - PuppetLanguageServer::PuppetHelper.load_default_types_async + if featureflag?('puppetstrings') + log_message(:info, 'Preloading Default metadata (Async)...') + PuppetLanguageServer::PuppetHelper.load_default_aggregate_async - log_message(:info, 'Preloading Facter (Async)...') - PuppetLanguageServer::FacterHelper.load_facts_async + log_message(:info, 'Preloading Facter (Async)...') + PuppetLanguageServer::FacterHelper.load_facts_async + else + log_message(:info, 'Preloading Puppet Types (Async)...') + PuppetLanguageServer::PuppetHelper.load_default_types_async - log_message(:info, 'Preloading Functions (Async)...') - PuppetLanguageServer::PuppetHelper.load_default_functions_async + log_message(:info, 'Preloading Facter (Async)...') + PuppetLanguageServer::FacterHelper.load_facts_async - log_message(:info, 'Preloading Classes (Async)...') - PuppetLanguageServer::PuppetHelper.load_default_classes_async + log_message(:info, 'Preloading Functions (Async)...') + PuppetLanguageServer::PuppetHelper.load_default_functions_async + + log_message(:info, 'Preloading Classes (Async)...') + PuppetLanguageServer::PuppetHelper.load_default_classes_async + end if PuppetLanguageServer::DocumentStore.store_has_module_metadata? || PuppetLanguageServer::DocumentStore.store_has_environmentconf? log_message(:info, 'Preloading Workspace (Async)...') diff --git a/spec/languageserver/integration/puppet-languageserver/sidecar_queue_spec.rb b/spec/languageserver/integration/puppet-languageserver/sidecar_queue_spec.rb index 1e9fc640..21e0d1e0 100644 --- a/spec/languageserver/integration/puppet-languageserver/sidecar_queue_spec.rb +++ b/spec/languageserver/integration/puppet-languageserver/sidecar_queue_spec.rb @@ -62,6 +62,28 @@ def exitstatus end describe '#execute_sync' do + context 'default_aggregate action' do + let(:action) { 'default_aggregate' } + + it 'should deserialize the json, import into the cache and assert default classes are loaded' do + fixture = PuppetLanguageServer::Sidecar::Protocol::AggregateMetadata.new + fixture.append!(random_sidecar_puppet_class) + fixture.append!(random_sidecar_puppet_function) + fixture.append!(random_sidecar_puppet_type) + sidecar_response = [fixture.to_json, 'stderr', SuccessStatus.new] + + expect(subject).to receive(:run_sidecar).and_return(sidecar_response) + expect(PuppetLanguageServer::PuppetHelper).to receive(:assert_default_classes_loaded) + expect(PuppetLanguageServer::PuppetHelper).to receive(:assert_default_functions_loaded) + expect(PuppetLanguageServer::PuppetHelper).to receive(:assert_default_types_loaded) + + subject.execute_sync(action, []) + expect(cache.object_by_name(:class, fixture.classes[0].key)).to_not be_nil + expect(cache.object_by_name(:function, fixture.functions[0].key)).to_not be_nil + expect(cache.object_by_name(:type, fixture.types[0].key)).to_not be_nil + end + end + context 'default_classes action' do let(:action) { 'default_classes' }