From 0b0d8be18a5229e08487a220b036c4fdbb358346 Mon Sep 17 00:00:00 2001 From: Daniel Miranda Date: Mon, 18 Jul 2016 07:50:04 -0300 Subject: [PATCH 1/5] Update ModuleResult tree-walking methods - Slightly refactor pre-order - Add level order, which should be faster when fetching from the database, by making one query per-level of the tree, instead of possibly one per-node --- app/models/module_result.rb | 37 ++++++++- spec/models/module_result_spec.rb | 125 +++++++++++++++++------------- 2 files changed, 102 insertions(+), 60 deletions(-) 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/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 From c7c1e73cb49d8a51b622e389628118b2d51e5dd9 Mon Sep 17 00:00:00 2001 From: Daniel Miranda Date: Mon, 18 Jul 2016 07:50:12 -0300 Subject: [PATCH 2/5] Optimize TreeMetricResult#descendant_values The previous implementation was incredibly sub-optimal. It can be easily replaced with a join on the parent_id. --- app/models/tree_metric_result.rb | 8 ++++---- .../tree_metric_results_controller_spec.rb | 6 ++---- spec/models/tree_metric_result_spec.rb | 14 ++++---------- 3 files changed, 10 insertions(+), 18 deletions(-) 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/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/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 From 20218aa7f51589c6a280a4b46e6f751a3eb7c0a5 Mon Sep 17 00:00:00 2001 From: Daniel Miranda Date: Tue, 19 Jul 2016 09:19:50 -0300 Subject: [PATCH 3/5] Rewrite aggregation processing to be more efficient Previously aggregation worked by traversing the modules tree in pre-order. But to ensure that children are aggregated before their parents, we can relax that order a bit to just processes all the results on the same level before all of those on a level above it (the topmost level consisting of the root). This allows fetching much more results at once and significantly reduce t he number of trips to the database - from a number proportional to the number of nodes, to exactly and no more than the maximum depth of the tree. It also makes it much easier to accumulate the created tree metric results to be created all at once. That also saves a huge number of trips to the database. Using the aggregation performance tests, in my development machine the average time - combined with the indexing changes that were previously made - went from around 200s to <20s. Regarding tests: a complete refactor was necessary, and made possible by the module results tree factory. The tests ended up much cleaner and arguably better, as they can verify the actual values being aggregated while mocking only the necessary data accesses. --- lib/processor/aggregator.rb | 86 +++++++++++++++---------- spec/lib/processor/aggregator_spec.rb | 90 +++++++++++++-------------- 2 files changed, 97 insertions(+), 79 deletions(-) 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/lib/processor/aggregator_spec.rb b/spec/lib/processor/aggregator_spec.rb index 7163767..316ddc4 100644 --- a/spec/lib/processor/aggregator_spec.rb +++ b/spec/lib/processor/aggregator_spec.rb @@ -4,61 +4,59 @@ 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) } + let(:root_module_result) { FactoryGirl.build(:module_result, :without_results) } + let(:processing) { FactoryGirl.build(:processing, root_module_result: root_module_result) } + let(:tree_metric_configuration) { FactoryGirl.build(:metric_configuration, :with_id, :flog, + aggregation_form: :SUM) } + 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 + } + let(:module_results_tree) { FactoryGirl.build(:module_results_tree, root: root_module_result) } + let(:leaf_value) { 1.0 } - 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)] } + context 'with tree native metrics' do + before :each do + levels = module_results_tree.levels + root_module_result.stubs(:descendants_by_level).returns(levels) - 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) + # Add metric results to all the leaves of the tree + levels.last.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 - it 'is expected to aggregate results' do - Processor::Aggregator.task(context) - end + TreeMetricResult.expects(:import!) 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)] } + it 'is expected to aggregate results to all levels of the tree' do + Processor::Aggregator.task(context) - 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 + 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 - it 'is expected to aggregate results' do - Processor::Aggregator.task(context) - 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 - module_results_tree.levels.last) + + Processor::Aggregator.task(context) + + expect(non_leaves.map(&:tree_metric_results)).to all(be_empty) end end end From 0fcb09eb9a1a64ebb891a8639f60aa5b52dd8348 Mon Sep 17 00:00:00 2001 From: Daniel Miranda Date: Tue, 19 Jul 2016 09:28:38 -0300 Subject: [PATCH 4/5] Remove unused MetricResultAggregator class It is no longer used since the new aggregation processing collects the tree metric results itself, making the auxiliary logic to find out whether a node already has a result for a metric unnecessary. --- lib/metric_result_aggregator.rb | 11 ---- spec/lib/metric_result_aggregator_spec.rb | 66 ----------------------- 2 files changed, 77 deletions(-) delete mode 100644 lib/metric_result_aggregator.rb delete mode 100644 spec/lib/metric_result_aggregator_spec.rb 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/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 From 8137ecc8e6706f376f1468f8e563d5f53e912140 Mon Sep 17 00:00:00 2001 From: Daniel Miranda Date: Fri, 22 Jul 2016 15:56:03 -0300 Subject: [PATCH 5/5] Update aggregation tests to verify all forms --- spec/lib/processor/aggregator_spec.rb | 85 +++++++++++++++++++++------ 1 file changed, 67 insertions(+), 18 deletions(-) diff --git a/spec/lib/processor/aggregator_spec.rb b/spec/lib/processor/aggregator_spec.rb index 316ddc4..a4f21db 100644 --- a/spec/lib/processor/aggregator_spec.rb +++ b/spec/lib/processor/aggregator_spec.rb @@ -4,10 +4,13 @@ describe Processor::Aggregator do describe 'methods' do describe 'task' do - let(:root_module_result) { FactoryGirl.build(:module_result, :without_results) } - let(:processing) { FactoryGirl.build(:processing, root_module_result: root_module_result) } + 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: :SUM) } + 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) { @@ -17,16 +20,15 @@ end end } - let(:module_results_tree) { FactoryGirl.build(:module_results_tree, root: root_module_result) } - let(:leaf_value) { 1.0 } context 'with tree native metrics' do + let(:leaf_value) { 1.0 } + before :each do - levels = module_results_tree.levels - root_module_result.stubs(:descendants_by_level).returns(levels) + root_module_result.expects(:descendants_by_level).returns(module_results_tree.levels) # Add metric results to all the leaves of the tree - levels.last.each do |module_result| + 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) @@ -35,15 +37,62 @@ TreeMetricResult.expects(:import!) end - it 'is expected to aggregate results to all levels of the tree' do - Processor::Aggregator.task(context) + 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 + # 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 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 MAX aggregation form' do + let(:aggregation_form) { :MAX } + let(:max_value) { leaf_value + 1 } + + before :each do + # 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 to the root 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 + expect(root_module_result.tree_metric_results).to all(have_attributes(value: max_value)) end end end @@ -52,11 +101,11 @@ let(:native_metrics) { [hotspot_metric_configuration] } it 'is expected not to aggregate any results' do - non_leaves = (module_results_tree.all - module_results_tree.levels.last) + non_leaves = (module_results_tree.all - leaves) Processor::Aggregator.task(context) - expect(non_leaves.map(&:tree_metric_results)).to all(be_empty) + expect(non_leaves).to all(have_attributes(tree_metric_results: be_empty)) end end end