From e222b5d78832b9ab05e5bf989cf07dd77ac786d6 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Tue, 28 Feb 2023 20:16:04 -0500 Subject: [PATCH] Fix sorting tests. Drop STRICT. We already follow these cases. I also cant remember why that case is significant / what it is trying to tell Drop a number of if cases that I'm pretty sure are deterministic I also documented why the non-optimal values are returned. use a common ranking function: RANK_SORT --- test/concerns/sort_by_ancestry_test.rb | 110 ++++++++++++++++++------- 1 file changed, 80 insertions(+), 30 deletions(-) diff --git a/test/concerns/sort_by_ancestry_test.rb b/test/concerns/sort_by_ancestry_test.rb index ea1c85f2..03abf5a5 100644 --- a/test/concerns/sort_by_ancestry_test.rb +++ b/test/concerns/sort_by_ancestry_test.rb @@ -1,12 +1,11 @@ require_relative '../environment' class SortByAncestryTest < ActiveSupport::TestCase - # in a perfect world, we'd only follow the CORRECT=true case - # but when not enough information is available, the STRICT=true case is good enough - # - # these flags are to allow multiple values for correct for tests + # In a perfect world, we'd only follow the CORRECT=true case + # This highlights where/why a non-correct sorting order is returned CORRECT = (ENV["CORRECT"] == "true") - STRICT = (ENV["STRICT"] == "true") + + RANK_SORT = -> (a, b) { a.rank <=> b.rank } # tree is of the form: # - n1 @@ -31,6 +30,9 @@ def build_tree(model) [n1, n2, n3, n4, n5, n6] end + # nodes among the same parent have an ambigious order + # so they keep the same order as input + # also note, parent nodes do come in first def test_sort_by_ancestry_full_tree AncestryTestDatabase.with_model do |model| n1, n2, n3, n4, n5, n6 = build_tree(model) @@ -40,6 +42,11 @@ def test_sort_by_ancestry_full_tree end end + # tree is of the form: + # - x + # - x + # - n3 + # - n4 def test_sort_by_ancestry_no_parents_siblings AncestryTestDatabase.with_model do |model| _, _, n3, n4, _, _ = build_tree(model) @@ -48,6 +55,8 @@ def test_sort_by_ancestry_no_parents_siblings end end + # TODO: thinking about dropping this one + # only keep if we can find a def test_sort_by_ancestry_no_parents_same_level AncestryTestDatabase.with_model do |model| _, _, n3, n4, n5, _ = build_tree(model) @@ -64,15 +73,30 @@ def test_sort_by_ancestry_partial_tree end end + # - n1 + # - x + # - n4 + # - n5 + # + # Issue: + # + # since the nodes are not at the same level, we don't have + # a way to know if n4 comes before or after n5 + # + # n1 will always come first since it is a parent of both + # Since we don't have n2, to bring n4 before n5, we leave in input order + + # TODO: thinking about dropping this test + # can't think of a way that these records would come back with sql order def test_sort_by_ancestry_missing_parent_middle_of_tree AncestryTestDatabase.with_model do |model| n1, _, _, n4, n5, _ = build_tree(model) records = model.sort_by_ancestry([n5, n4, n1]) - if (!CORRECT) && (STRICT || records[1] == n5) - assert_equal [n1, n5, n4].map(&:id), records.map(&:id) - else + if CORRECT assert_equal [n1, n4, n5].map(&:id), records.map(&:id) + else + assert_equal [n1, n5, n4].map(&:id), records.map(&:id) end end end @@ -87,6 +111,7 @@ def test_sort_by_ancestry_missing_parent_middle_of_tree # @returns [Array] list of ranked nodes def build_ranked_tree(model) # inflate the node id to test id wrap around edge cases + # NODES=4..9 seem like edge cases ENV["NODES"].to_i.times { model.create!.destroy } if ENV["NODES"] n1 = model.create!(:rank => 0) @@ -100,12 +125,14 @@ def build_ranked_tree(model) [n1, n2, n3, n4, n5, n6] end + # TODO: thinking about dropping this one + # Think we need to assume that best effort was done in the database: + # ordered_by_ancestry_and(:id => :desc) or order(:ancestry).order(:id => :desc) def test_sort_by_ancestry_with_block_full_tree AncestryTestDatabase.with_model :extra_columns => {:rank => :integer} do |model| n1, n2, n3, n4, n5, n6 = build_ranked_tree(model) - sort = -> (a, b) { a.rank <=> b.rank } - records = model.sort_by_ancestry(model.all.order(:id).reverse, &sort) + records = model.sort_by_ancestry(model.all.order(:id => :desc), &RANK_SORT) assert_equal [n1, n5, n3, n2, n4, n6].map(&:id), records.map(&:id) end end @@ -123,22 +150,33 @@ def test_sort_by_ancestry_with_block_full_tree_sql_sort def test_sort_by_ancestry_with_block_all_parents_some_children AncestryTestDatabase.with_model :extra_columns => {:rank => :integer} do |model| n1, n2, _, _, n5, _ = build_ranked_tree(model) - sort = -> (a, b) { a.rank <=> b.rank } - assert_equal [n1, n5, n2].map(&:id), model.sort_by_ancestry([n1, n2, n5], &sort).map(&:id) + assert_equal [n1, n5, n2].map(&:id), model.sort_by_ancestry([n1, n2, n5], &RANK_SORT).map(&:id) end end - # seems the best we can do is to have [5,3] + [4,6] - # if we follow input order, we can end up with either result - # a) n3 moves all the way to the right or b) n5 moves all the way to the left - # TODO: find a way to rank missing nodes + # It is tricky when we are using ruby to sort nodes and the parent + # nodes (i.e.: n1, n2) are not in ruby to be sorted. We either sort + # them by input order or by id order. + # + # - x (0) + # - n5 (0) + # - n3 (3) + # - x (1) + # - n4 (0) + # - n6 (1) + # We can sort [n5, n3] + [n4, n6] + # a) n3 moves all the way to the right to join n5 OR + # b) n5 moves all the way to the left to join n3 + # Issue: + # we do not know if the parent of n5 (n1) comes before or after the parent of n4 (n2) + # So they should stay in their original order + # But again, it is indeterministic which way the 2 pairs go def test_sort_by_ancestry_with_block_no_parents_all_children AncestryTestDatabase.with_model :extra_columns => {:rank => :integer} do |model| _, _, n3, n4, n5, n6 = build_ranked_tree(model) - sort = -> (a, b) { a.rank <=> b.rank } - records = model.sort_by_ancestry([n3, n4, n5, n6], &sort) + records = model.sort_by_ancestry([n3, n4, n5, n6], &RANK_SORT) if CORRECT || records[0] == n5 assert_equal [n5, n3, n4, n6].map(&:id), records.map(&:id) else @@ -147,34 +185,46 @@ def test_sort_by_ancestry_with_block_no_parents_all_children end end - # TODO: nodes need to follow original ordering + # - x (0) + # - x + # - n3 (3) + # - n2 (1) + # - n4 (0) + # - x + # Issue: n2 will always go before n4. + # But n1 is not available to put n3 before the n2 tree. + # not sure why it doesn't follow the input order + # # NOTE: even for partial trees, if the input records are ranked, the output works def test_sort_by_ancestry_with_sql_sort_paginated_missing_parents_and_children AncestryTestDatabase.with_model :extra_columns => {:rank => :integer} do |model| _, n2, n3, n4, _, _ = build_ranked_tree(model) records = model.sort_by_ancestry([n2, n4, n3]) - if (!CORRECT) && (STRICT || records[0] == n2) - assert_equal [n2, n4, n3].map(&:id), records.map(&:id) - else + if CORRECT assert_equal [n3, n2, n4].map(&:id), records.map(&:id) + else + assert_equal [n2, n4, n3].map(&:id), records.map(&:id) end end end - # in a perfect world, the second case would be matched - # but since presorting is not used, the best we can assume from input order is that n1 > n2 - # TODO: find a way to rank missing nodes + # same as above but using sort block + # - x (0) + # - x + # - n3 (3) + # - n2 (1) + # - n4 (0) + # - n5 def test_sort_by_ancestry_with_block_paginated_missing_parents_and_children AncestryTestDatabase.with_model :extra_columns => {:rank => :integer} do |model| _, n2, n3, n4, _, _ = build_ranked_tree(model) - sort = -> (a, b) { a.rank <=> b.rank } - records = model.sort_by_ancestry([n2, n4, n3], &sort) - if (!CORRECT) && (STRICT || records[0] == n2) - assert_equal [n2, n4, n3].map(&:id), records.map(&:id) - else + records = model.sort_by_ancestry([n2, n4, n3], &RANK_SORT) + if CORRECT assert_equal [n3, n2, n4].map(&:id), records.map(&:id) + else + assert_equal [n2, n4, n3].map(&:id), records.map(&:id) end end end