diff --git a/app/models/module_result.rb b/app/models/module_result.rb index 25c785a..be2f62e 100644 --- a/app/models/module_result.rb +++ b/app/models/module_result.rb @@ -31,13 +31,33 @@ def to_json(options={}) end def pre_order - root = self - root = root.parent until root.parent.nil? - @pre_order ||= pre_order_traverse(root).to_a + return @pre_order unless @pre_order.nil? + return processing.root_module_result.pre_order unless parent.nil? + + @pre_order = pre_order_traverse(self).to_a + end + + def level_order + return @level_order unless @level_order.nil? + return processing.root_module_result.level_order unless parent.nil? + + @level_order = descendants_by_level.flatten end def descendants - @descendants ||= pre_order_traverse(self).to_a + @descendants ||= descendants_by_level.flatten + end + + def descendants_by_level + level = [self] + descendants = [] + + until level.empty? do + descendants << level + level = fetch_all_children(level) + end + + descendants end def descendant_hotspot_metric_results @@ -66,4 +86,13 @@ def pre_order_traverse(module_result, &block) end end end + + def fetch_all_children(parents) + self.class. + where(parent_id: parents.map(&:id)). + eager_load(:kalibro_module, :tree_metric_results). + to_a + end + + end diff --git a/app/models/tree_metric_result.rb b/app/models/tree_metric_result.rb index 0b2747b..1a85604 100644 --- a/app/models/tree_metric_result.rb +++ b/app/models/tree_metric_result.rb @@ -14,10 +14,10 @@ def has_grade? def grade; self.range.grade; end def descendant_values - module_result.children.map { |child| - metric_result = child.tree_metric_result_for(self.metric) - metric_result.value if metric_result - }.compact + self.class. + joins(:module_result). + where('module_results.parent_id' => module_result_id). + pluck(:value) end def as_json(options={}) diff --git a/lib/metric_result_aggregator.rb b/lib/metric_result_aggregator.rb deleted file mode 100644 index 168b29f..0000000 --- a/lib/metric_result_aggregator.rb +++ /dev/null @@ -1,11 +0,0 @@ -class MetricResultAggregator - def self.aggregated_value(metric_result) - values = DescriptiveStatistics::Stats.new(metric_result.descendant_values) - if metric_result.value.nil? && !values.empty? - aggregation_form = metric_result.metric_configuration.aggregation_form.to_s.downcase - values.send(aggregation_form) - else - metric_result.value - end - end -end diff --git a/lib/processor/aggregator.rb b/lib/processor/aggregator.rb index 1a68bbb..36e39ff 100644 --- a/lib/processor/aggregator.rb +++ b/lib/processor/aggregator.rb @@ -1,54 +1,74 @@ -require 'metric_result_aggregator' - module Processor class Aggregator < ProcessingStep - protected + def self.task(context) + # Only bother processing Tree metric configurations + # Do nothing if there are none + metric_configurations = extract_tree_metric_configurations(context) + return if metric_configurations.empty? - def self.task(runner) - @native_metrics = runner.native_metrics - set_all_metrics - aggregate(runner.processing.root_module_result.pre_order) - end + # Find all the descendants of the root at once. + root = context.processing.root_module_result + descendants_by_level = root.descendants_by_level + # Keep a hash to allow looking up parents by id without going to the DB again + module_results_by_id = module_results_by_id(descendants_by_level) - def self.state - "AGGREGATING" - end + new_tree_metric_results = [] - private + # Process levels bottom-up to ensure every descendants has been aggregated already when a module result is + # processed + descendants_by_level.reverse_each do |level| + # Group module results in each level by their parents and aggregate the results for the group, to then be + # written to the parent + level.group_by(&:parent_id).each do |parent_id, children| + next if parent_id.nil? + parent = module_results_by_id[parent_id] - def self.aggregate(pre_order_module_results) - # The upper nodes of the tree need the children to be calculated first, so we reverse the pre_order - pre_order_module_results.reverse_each do | module_result_child | + metric_configurations.each do |mc| + # Ensure we won't try to aggregate into something of an unexpected granularity + next if parent.kalibro_module.granularity < mc.metric.scope - already_calculated_metrics = module_result_child.tree_metric_results.map { |tree_metric_result| tree_metric_result.metric} + # Don't create a result if the parent already has a result for this metric + next if parent.tree_metric_results.any? { |tmr| tmr.metric_configuration_id == mc.id } - remaining_metrics = @all_metrics.reject { |metric| already_calculated_metrics.include?(metric) } + # Get all the metric results for this metric configuration from all the module results in the group + tree_metric_results = children.map { |module_result| + module_result.tree_metric_results.find { |tmr| tmr.metric_configuration_id == mc.id } + }.compact - remaining_metrics.each do |metric| - if module_result_child.kalibro_module.granularity >= KalibroClient::Entities::Miscellaneous::Granularity.new(metric.scope.to_s.to_sym) - tree_metric_result = TreeMetricResult.new(metric: metric, module_result: module_result_child, metric_configuration_id: metric_configuration(metric).id) - tree_metric_result.value = MetricResultAggregator.aggregated_value(tree_metric_result) - tree_metric_result.save + # Aggregate the results and save + new_value = aggregate_values(tree_metric_results, mc) + new_tree_metric_results << parent.tree_metric_results.build(metric_configuration_id: mc.id, value: new_value) end end end + + TreeMetricResult.import!(new_tree_metric_results) end - def self.set_all_metrics - @all_metrics = [] - @native_metrics.each_value do |metric_configurations| - metric_configurations.each do |metric_configuration| - @all_metrics << metric_configuration.metric - end - end + def self.state + "AGGREGATING" + end + + def self.extract_tree_metric_configurations(context) + context.native_metrics.values.flatten.reject { |mc| mc.metric.type != 'NativeMetricSnapshot' } end - def self.metric_configuration(metric) - @native_metrics.each_value do |metric_configurations| - metric_configurations.each do |metric_configuration| - return metric_configuration if metric_configuration.metric == metric + def self.module_results_by_id(descendants_by_level) + results = {} + descendants_by_level.each do |level| + level.each do |module_result| + results[module_result.id] = module_result end end + + results + end + + def self.aggregate_values(tree_metric_results, metric_configuration) + aggregation_form = metric_configuration.aggregation_form.to_s.downcase + DescriptiveStatistics::Stats.new(tree_metric_results.map(&:value)).send(aggregation_form) end + + private_class_method :extract_tree_metric_configurations, :module_results_by_id, :aggregate_values end end diff --git a/spec/controllers/tree_metric_results_controller_spec.rb b/spec/controllers/tree_metric_results_controller_spec.rb index adef093..b120836 100644 --- a/spec/controllers/tree_metric_results_controller_spec.rb +++ b/spec/controllers/tree_metric_results_controller_spec.rb @@ -9,10 +9,8 @@ let!(:module_result) { FactoryGirl.build(:module_result) } before :each do - module_result.expects(:tree_metric_result_for).with(metric_result.metric).returns(metric_result) - module_result.expects(:children).returns([module_result]) - metric_result.expects(:module_result).returns(module_result) - TreeMetricResult.expects(:find).with(metric_result.id).returns(metric_result) + MetricResult.expects(:find).with(metric_result.id).returns(metric_result) + metric_result.expects(:descendant_values).returns([metric_result.value]) end context 'json format' do diff --git a/spec/lib/metric_result_aggregator_spec.rb b/spec/lib/metric_result_aggregator_spec.rb deleted file mode 100644 index 202a99c..0000000 --- a/spec/lib/metric_result_aggregator_spec.rb +++ /dev/null @@ -1,66 +0,0 @@ -require 'rails_helper' -require 'metric_result_aggregator' - -describe MetricResultAggregator, :type => :model do - describe 'method' do - let!(:metric_configuration) { FactoryGirl.build(:metric_configuration) } - let!(:another_metric_configuration) { FactoryGirl.build(:another_metric_configuration) } - let!(:sum_metric_configuration) { FactoryGirl.build(:sum_metric_configuration) } - let!(:maximum_metric_configuration) { FactoryGirl.build(:maximum_metric_configuration) } - let!(:minimum_metric_configuration) { FactoryGirl.build(:minimum_metric_configuration) } - - describe 'aggregated_value' do - let!(:stats) { DescriptiveStatistics::Stats.new([2, 4, 6]) } - - before :each do - subject.expects(:descendant_values).returns(stats) - end - - context 'when value is nil and the values array is not empty' do - subject { FactoryGirl.build(:tree_metric_result, module_result: FactoryGirl.build(:module_result)) } - - it 'should calculate the mean value of the values array' do - KalibroClient::Entities::Configurations::MetricConfiguration.expects(:find). - with(subject.metric_configuration_id).returns(metric_configuration) - expect(MetricResultAggregator.aggregated_value(subject)).to eq(4.0) - end - - it 'should count the values of array' do - KalibroClient::Entities::Configurations::MetricConfiguration.expects(:find). - with(subject.metric_configuration_id).returns(another_metric_configuration) - expect(MetricResultAggregator.aggregated_value(subject)).to eq(3) - end - - it 'should sum the values of array' do - KalibroClient::Entities::Configurations::MetricConfiguration.expects(:find). - with(subject.metric_configuration_id).returns(sum_metric_configuration) - expect(MetricResultAggregator.aggregated_value(subject)).to eq(12) - end - - it 'should minimize the values of array' do - KalibroClient::Entities::Configurations::MetricConfiguration.expects(:find). - with(subject.metric_configuration_id).returns(minimum_metric_configuration) - expect(MetricResultAggregator.aggregated_value(subject)).to eq(2) - end - - it 'should maximize the values of array' do - KalibroClient::Entities::Configurations::MetricConfiguration.expects(:find). - with(subject.metric_configuration_id).returns(maximum_metric_configuration) - expect(MetricResultAggregator.aggregated_value(subject)).to eq(6) - end - - after :each do - Rails.cache.clear # This test depends on metric configuration - end - end - - context 'when the metric_results are not from a leaf module' do - subject { FactoryGirl.build(:tree_metric_result_with_value) } - - it 'should return the value' do - expect(MetricResultAggregator.aggregated_value(subject)).to eq(subject.value) - end - end - end - end -end diff --git a/spec/lib/processor/aggregator_spec.rb b/spec/lib/processor/aggregator_spec.rb index 7163767..a4f21db 100644 --- a/spec/lib/processor/aggregator_spec.rb +++ b/spec/lib/processor/aggregator_spec.rb @@ -4,63 +4,110 @@ describe Processor::Aggregator do describe 'methods' do describe 'task' do - let(:kalibro_configuration) { FactoryGirl.build(:kalibro_configuration) } - let!(:code_dir) { "/tmp/test" } - let!(:repository) { FactoryGirl.build(:repository, scm_type: "GIT", kalibro_configuration: kalibro_configuration, code_directory: code_dir) } - let!(:root_module_result) { FactoryGirl.build(:module_result) } - let!(:processing) { FactoryGirl.build(:processing, repository: repository, root_module_result: root_module_result) } - let!(:compound_metric_configurations) { [FactoryGirl.build(:compound_metric_configuration)] } - let!(:context) { FactoryGirl.build(:context, repository: repository, processing: processing) } - - context 'when the module result tree has been well-built' do - context 'with a linear hierarchy' do - let!(:child) { FactoryGirl.build(:module_result_class_granularity, parent: root_module_result) } - let!(:native_metric_configurations) { - [FactoryGirl.build(:metric_configuration, id: 1), - FactoryGirl.build(:metric_configuration, metric: FactoryGirl.build(:loc_metric), id: 2)] - } - let!(:native_metrics) { { "Analizo" => native_metric_configurations } } - let!(:all_metrics) { [native_metric_configurations.first.metric, native_metric_configurations.last.metric] } - let!(:tree_metric_results) { [FactoryGirl.build(:tree_metric_result, :with_value, metric_configuration: native_metric_configurations.first, metric: native_metric_configurations.first.metric), - FactoryGirl.build(:tree_metric_result, :with_value, metric_configuration: native_metric_configurations.last, metric: native_metric_configurations.last.metric)] } + let(:module_results_tree) { FactoryGirl.build(:module_results_tree, :with_id) } + let(:root_module_result) { module_results_tree.root } + let(:leaves) { module_results_tree.levels.last } + let(:processing) { root_module_result.processing } + let(:aggregation_form) { :SUM } + let(:tree_metric_configuration) { FactoryGirl.build(:metric_configuration, :with_id, :flog, + aggregation_form: aggregation_form) } + let(:hotspot_metric_configuration) { FactoryGirl.build(:hotspot_metric_configuration, :with_id) } + let(:native_metrics) { [tree_metric_configuration, hotspot_metric_configuration] } + let(:context) { + FactoryGirl.build(:context, processing: processing).tap do |context| + native_metrics.each do |mc| + context.native_metrics[mc.metric.metric_collector_name] << mc + end + end + } + + context 'with tree native metrics' do + let(:leaf_value) { 1.0 } + + before :each do + root_module_result.expects(:descendants_by_level).returns(module_results_tree.levels) + + # Add metric results to all the leaves of the tree + leaves.each do |module_result| + module_result.tree_metric_results.build(metric_configuration_id: tree_metric_configuration.id, + value: leaf_value) + module_result.hotspot_metric_results.build(metric_configuration_id: hotspot_metric_configuration.id) + end + + TreeMetricResult.expects(:import!) + end + + context 'with SUM aggregation form' do + it 'is expected to aggregate results to all levels of the tree' do + Processor::Aggregator.task(context) + + expected_value = leaf_value + module_results_tree.levels.reverse_each do |level| + tree_metric_results = level.map(&:tree_metric_results).flatten + expect(tree_metric_results).to all(have_attributes(metric_configuration_id: tree_metric_configuration.id, + value: expected_value)) + expected_value *= module_results_tree.width + end + end + end + + context 'with MEAN aggregation form' do + let(:aggregation_form) { :MEAN } + + it 'is expected to aggregate results to all levels of the tree' do + Processor::Aggregator.task(context) + + tree_metric_results = module_results_tree.all.map(&:tree_metric_results).flatten + expect(tree_metric_results).to all(have_attributes(value: leaf_value)) + end + end + + context 'with MIN aggregation form' do + let(:aggregation_form) { :MIN } + let(:min_value) { leaf_value - 1 } before :each do - context.native_metrics = native_metrics - child.expects(:tree_metric_results).returns(tree_metric_results) - root_module_result.expects(:pre_order).returns([root_module_result, child]) - TreeMetricResult.any_instance.expects(:save) + # Set only one tree_metric_result to the minimum value + module_result = leaves.first + module_result.tree_metric_results.first.value = min_value end - it 'is expected to aggregate results' do + it 'is expected to aggregate results to the root of the tree' do Processor::Aggregator.task(context) + + expect(root_module_result.tree_metric_results).to all(have_attributes(value: min_value)) end end - context 'with a multi-level package hierarchy' do - let!(:parent) { FactoryGirl.build(:module_result, :package, parent: root_module_result) } - let!(:child) { FactoryGirl.build(:module_result, :package, parent: parent) } - let!(:another_child) { FactoryGirl.build(:module_result, :package, parent: parent) } - let!(:native_metric_configurations) { - [FactoryGirl.build(:metric_configuration, metric: FactoryGirl.build(:maintainability_metric), id: 1)] - } - let!(:native_metrics) { { "Radon" => native_metric_configurations } } - let!(:all_metrics) { [native_metric_configurations.first.metric] } - let!(:tree_metric_results) { [FactoryGirl.build(:tree_metric_result, :with_value, metric_configuration: native_metric_configurations.first, metric: native_metric_configurations.first.metric)] } + context 'with MAX aggregation form' do + let(:aggregation_form) { :MAX } + let(:max_value) { leaf_value + 1 } before :each do - context.native_metrics = native_metrics - child.expects(:tree_metric_results).returns(tree_metric_results) - another_child.expects(:tree_metric_results).returns(tree_metric_results) - root_module_result.expects(:pre_order).returns([root_module_result, parent, child, another_child]) - TreeMetricResult.any_instance.expects(:save).twice - MetricResultAggregator.expects(:aggregated_value).twice + # Set only one tree_metric_result to the maximum value + module_result = leaves.first + module_result.tree_metric_results.first.value = max_value end - it 'is expected to aggregate results' do + it 'is expected to aggregate results to the root of the tree' do Processor::Aggregator.task(context) + + expect(root_module_result.tree_metric_results).to all(have_attributes(value: max_value)) end end end + + context 'without tree native metrics' do + let(:native_metrics) { [hotspot_metric_configuration] } + + it 'is expected not to aggregate any results' do + non_leaves = (module_results_tree.all - leaves) + + Processor::Aggregator.task(context) + + expect(non_leaves).to all(have_attributes(tree_metric_results: be_empty)) + end + end end describe 'state' do diff --git a/spec/models/module_result_spec.rb b/spec/models/module_result_spec.rb index 8ae6dbb..252d63d 100644 --- a/spec/models/module_result_spec.rb +++ b/spec/models/module_result_spec.rb @@ -64,10 +64,10 @@ end end - describe 'method' do - describe 'tree_metric_result_for' do - subject { FactoryGirl.build(:module_result) } + describe 'methods' do + subject { FactoryGirl.build(:module_result) } + describe 'tree_metric_result_for' do let(:metric_result) {subject.tree_metric_results.first} before :each do @@ -90,9 +90,9 @@ end describe 'find_by_module_and_processing' do - let!(:kalibro_module) { FactoryGirl.build(:kalibro_module) } - let!(:processing) { FactoryGirl.build(:processing) } - let!(:module_result) { FactoryGirl.build(:module_result) } + let(:kalibro_module) { FactoryGirl.build(:kalibro_module) } + let(:processing) { FactoryGirl.build(:processing) } + let(:module_result) { FactoryGirl.build(:module_result) } before :each do name_filtered_results = Object.new @@ -116,8 +116,6 @@ end describe 'to_json' do - subject { FactoryGirl.build(:module_result) } - it 'is expected to add the kalibro_module to the JSON' do expect(JSON.parse(subject.to_json)['kalibro_module']).to eq(JSON.parse(subject.kalibro_module.to_json)) end @@ -128,79 +126,94 @@ expect(JSON.parse(subject.to_json)['height']).to eq(JSON.parse(subject.to_json)['height']) end end + end + + describe 'tree-walking methods' do + let(:module_results_tree) { FactoryGirl.build(:module_results_tree, :with_id, height: 3, width: 2) } + let(:module_results) { module_results_tree.all } + let(:non_root_module_results) { module_results - [module_results_tree.root] } + subject { module_results_tree.root } + + before :each do + # Ensure fetching the children of the last level is handled (and returns an empty list) + levels_with_last = module_results_tree.levels + [[]] + levels_with_last.each_cons(2) do |prev_level, level| + ModuleResult.any_instance.stubs(:fetch_all_children).with(prev_level).returns(level) + end + end describe 'pre_order' do - context 'when it does not have children' do - before :each do - subject.expects(:children).returns([]) + let(:pre_order) { module_results.values_at(0, 1, 3, 4, 2, 5, 6) } + + context 'when it is at the root' do + context 'when there are children' do + it 'is expected to return an array with the pre order tree traversal' do + expect(subject.pre_order).to eq(pre_order) + end end - it 'is expected to return an array with only itself' do - expect(subject.pre_order).to eq([subject]) + + context 'when there are no children' do + let(:module_results_tree) { FactoryGirl.build(:module_results_tree, :with_id, height: 1) } + + it 'is expected to return an array with only itself' do + expect(subject.pre_order).to eq([subject]) + end end end - context 'when it does have children' do - let!(:child_1) { FactoryGirl.build(:module_result, id: 1) } - let!(:child_2) { FactoryGirl.build(:module_result, id: 2) } - let!(:grandchild_1) { FactoryGirl.build(:module_result, id: 3) } - let!(:grandchild_2) { FactoryGirl.build(:module_result, id: 4) } - let!(:grandchild_3) { FactoryGirl.build(:module_result, id: 5) } - let!(:grandchild_4) { FactoryGirl.build(:module_result, id: 6) } - - before :each do - subject.expects(:children).returns([child_1, child_2]) - child_1.expects(:children).returns([grandchild_1, grandchild_2]) - child_2.expects(:children).returns([grandchild_3, grandchild_4]) - grandchild_1.expects(:children).returns([]) - grandchild_2.expects(:children).returns([]) - grandchild_3.expects(:children).returns([]) - grandchild_4.expects(:children).returns([]) - end - it 'is expected to return an array with the pre order tree traversal' do - expect(subject.pre_order).to eq([subject, child_1, grandchild_1, grandchild_2, child_2, grandchild_3, grandchild_4]) + context 'when it is not at the root' do + it 'is expected to return the pre order tree traversal from the root' do + expect(non_root_module_results.map(&:pre_order)).to all(eq(pre_order)) end end + end - context 'when it is not the root module result' do - let!(:root) { FactoryGirl.build(:module_result, id: 1)} - let!(:child) { FactoryGirl.build(:module_result, id: 2)} - before :each do - child.expects(:parent).twice.returns(root) - root.expects(:parent).returns(nil) - root.expects(:children).returns([child]) + describe 'level_order' do + let(:level_order) { module_results_tree.levels.flatten } + + context 'when it is at the root' do + context 'when there are children' do + it 'is expected to return the level order tree traversal ' do + expect(subject.level_order).to eq(level_order) + end end - it 'is expected to return the complete pre order tree traversal (from root)' do - expect(child.pre_order).to eq([root, child]) + + context 'when there are no children' do + let(:module_results_tree) { FactoryGirl.build(:module_results_tree, :with_id, height: 1) } + + it 'is expected to return an array with only itself' do + expect(subject.level_order).to eq([subject]) + end end end - end - describe 'descendants' do - subject { FactoryGirl.build(:module_result) } - let!(:pre_order_module_results) { [subject] } + context 'when it is not at the root' do + it 'is expected to return the level order tree traversal from the root' do + expect(non_root_module_results.map(&:level_order)).to all(eq(level_order)) + end + end + end - before :each do - subject.expects(:pre_order_traverse).with(subject).returns(pre_order_module_results.to_enum) + describe 'descendants_by_level' do + it 'is expected to return a list with the levels from top to bottom' do + expect(subject.descendants_by_level).to eq(module_results_tree.levels) end + end + describe 'descendants' do it 'is expected to return the descendant ModuleResults' do - expect(subject.descendants).to be_a(Array) - expect(subject.descendants).to eq(pre_order_module_results) + expect(subject.descendants).to be_a(Array).and contain_exactly(*module_results_tree.all) end end describe 'descendant_hotspot_metric_results' do - subject { FactoryGirl.build(:module_result) } - let!(:descendant_module_results) { [subject] } - before :each do - subject.expects(:descendants).returns(descendant_module_results) + subject.expects(:descendants).returns(module_results) end it 'is expected to return the Hotspot MetricResults related with the descendant ModuleResults ids' do - result_mock = mock - HotspotMetricResult.expects(:where).with(module_result_id: [subject.id]).returns(result_mock) - + result_mock = mock('hotspot_metric_results') + HotspotMetricResult.expects(:where).with(module_result_id: module_results.map(&:id)).returns(result_mock) expect(subject.descendant_hotspot_metric_results).to eq(result_mock) end end diff --git a/spec/models/tree_metric_result_spec.rb b/spec/models/tree_metric_result_spec.rb index 7272efd..74e9d04 100644 --- a/spec/models/tree_metric_result_spec.rb +++ b/spec/models/tree_metric_result_spec.rb @@ -100,23 +100,17 @@ describe 'descendant_values' do subject { FactoryGirl.build(:tree_metric_result) } - let(:son) { FactoryGirl.build(:tree_metric_result_with_value) } - let!(:module_result) { FactoryGirl.build(:module_result) } before :each do - subject.expects(:module_result).returns(module_result) - module_result.expects(:children).returns([module_result, module_result]) - module_result.expects(:tree_metric_result_for).at_least_once. - with(subject.metric).returns(son) + query = mock() + described_class.expects(:joins).with(:module_result).returns(query) + query.expects(:where).with('module_results.parent_id' => subject.module_result_id).returns(query) + query.expects(:pluck).with(:value).returns([2.0, 2.0]) end it "should return an array with all its children's values" do expect(subject.descendant_values).to eq([2.0, 2.0]) end - - it "should be a float values array" do - expect(subject.descendant_values).to be_a(Array) - end end end end