From e9a6b85ed43d1b6dd08c3d845cbfcf90baac073b Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Tue, 28 Mar 2023 21:42:49 -0400 Subject: [PATCH 1/6] Remove materialized path specific tests --- test/concerns/arrangement_test.rb | 60 ++++++++++++------------- test/concerns/orphan_strategies_test.rb | 19 ++++---- 2 files changed, 37 insertions(+), 42 deletions(-) diff --git a/test/concerns/arrangement_test.rb b/test/concerns/arrangement_test.rb index cdc8997e..8257334c 100644 --- a/test/concerns/arrangement_test.rb +++ b/test/concerns/arrangement_test.rb @@ -129,29 +129,22 @@ def test_ancestors_arrange_leaf_node def test_arrange_serializable AncestryTestDatabase.with_model :depth => 2, :width => 2 do |model, _roots| - if AncestryTestDatabase.materialized_path2? - result = [{"ancestry"=>'/', - "id"=>4, - "children"=> - [{"ancestry"=>"/4/", "id"=>6, "children"=>[]}, - {"ancestry"=>"/4/", "id"=>5, "children"=>[]}]}, - {"ancestry"=>'/', - "id"=>1, - "children"=> - [{"ancestry"=>"/1/", "id"=>3, "children"=>[]}, - {"ancestry"=>"/1/", "id"=>2, "children"=>[]}]}] - else - result = [{"ancestry"=>nil, - "id"=>4, - "children"=> - [{"ancestry"=>"4", "id"=>6, "children"=>[]}, - {"ancestry"=>"4", "id"=>5, "children"=>[]}]}, - {"ancestry"=>nil, - "id"=>1, - "children"=> - [{"ancestry"=>"1", "id"=>3, "children"=>[]}, - {"ancestry"=>"1", "id"=>2, "children"=>[]}]}] - end + col = model.ancestry_column + # materialized path 2 has a slash at the beginning and end + fmt = AncestryTestDatabase.materialized_path2? ? -> (a) { a ? "/#{a}/" : "/" } : -> (a) {a} + result = [{ + col=>fmt[nil], "id"=>4, "children"=> [{ + col=>fmt["4"], "id"=>6, "children" => [] + }, { + col=>fmt["4"], "id"=>5, "children" => [] + }] + }, { + col=>fmt[nil], "id"=>1, "children"=> [{ + col=>fmt["1"], "id"=>3, "children"=>[] + }, { + col=>fmt["1"], "id"=>2, "children"=>[] + }] + }] assert_equal model.arrange_serializable(order: "id desc"), result end @@ -160,15 +153,18 @@ def test_arrange_serializable def test_arrange_serializable_with_block AncestryTestDatabase.with_model :depth => 2, :width => 2 do |model, _roots| expected_result = [{ - "id"=>4, - "children"=> - [{"id"=>6}, - {"id"=>5}]}, - { - "id"=>1, - "children"=> - [{"id"=>3}, - {"id"=>2}]}] + "id"=>4, "children"=>[{ + "id"=>6 + }, { + "id"=>5 + }] + }, { + "id"=>1, "children"=> [{ + "id"=>3 + }, { + "id"=>2 + }] + }] result = model.arrange_serializable(order: "id desc") do |parent, children| out = {} out["id"] = parent.id diff --git a/test/concerns/orphan_strategies_test.rb b/test/concerns/orphan_strategies_test.rb index d9db2e0e..8474a735 100644 --- a/test/concerns/orphan_strategies_test.rb +++ b/test/concerns/orphan_strategies_test.rb @@ -55,17 +55,16 @@ def test_orphan_adopt_strategy n4 = model.create!(:parent => n2) #create child with parent=n2, depth = 2 n5 = model.create!(:parent => n4) #create child with parent=n4, depth = 3 n2.destroy # delete a node with desecendants - assert_equal(model.find(n3.id).parent,n1, "orphan's not parentified" ) - assert_equal(model.find(n5.id).ancestor_ids, [n1.id,n4.id], "ancestry integrity not maintained") + n3.reload + n5.reload + assert_equal n3.parent_id, n1.id, "orphan's not parentified" + assert_equal n5.ancestor_ids, [n1.id, n4.id], "ancestry integrity not maintained" n1.destroy # delete a root node with desecendants - if AncestryTestDatabase.materialized_path2? - assert_equal(model.find(n3.id).ancestry, model.ancestry_root, " new root node has no root ancestry string") - else - assert_nil(model.find(n3.id).ancestry," new root node has no empty ancestry string") - end - assert_equal(model.find(n3.id).valid?, true, " new root node is not valid") - assert_nil(model.find(n3.id).parent_id, " Children of the deleted root not rootfied") - assert_equal(model.find(n5.id).ancestor_ids, [n4.id], "ancestry integrity not maintained") + n3.reload + n5.reload + assert_nil n3.parent_id, " new root node should have no parent" + assert n3.valid?, " new root node is not valid" + assert_equal n5.ancestor_ids, [n4.id], "ancestry integrity not maintained" end end From eb3f6217d01405a5ee2bff607eaed6e571b9c425 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Tue, 28 Mar 2023 21:22:26 -0400 Subject: [PATCH 2/6] don't reference column ancestry in tests (use model.ancestry_column) --- test/concerns/has_ancestry_test.rb | 6 +++++- test/concerns/materialized_path_test.rb | 9 +++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/test/concerns/has_ancestry_test.rb b/test/concerns/has_ancestry_test.rb index 460e93ed..81b7d772 100644 --- a/test/concerns/has_ancestry_test.rb +++ b/test/concerns/has_ancestry_test.rb @@ -2,7 +2,11 @@ class HasAncestryTreeTest < ActiveSupport::TestCase def test_default_ancestry_column - AncestryTestDatabase.with_model do |model| + AncestryTestDatabase.with_model skip_ancestry: true, ancestry_column: :ancestry do |model| + model.class_eval do + # explicitly calling has_ancestry so we can be sure no args passed + has_ancestry + end assert_equal :ancestry, model.ancestry_column end end diff --git a/test/concerns/materialized_path_test.rb b/test/concerns/materialized_path_test.rb index 2c705f12..4d38cdf1 100644 --- a/test/concerns/materialized_path_test.rb +++ b/test/concerns/materialized_path_test.rb @@ -103,17 +103,18 @@ def test_ancestry_validation_exclude_self private def assert_ancestry(node, value, child: :skip, db: :value) + column_name = node.class.ancestry_column if value.nil? - assert_nil node.ancestry + assert_nil node.send(column_name) else - assert_equal value, node.ancestry + assert_equal value, node.send(column_name) end db = value if db == :value if db.nil? - assert_nil node.ancestry_in_database + assert_nil node.send("#{column_name}_in_database") else - assert_equal db, node.ancestry_in_database + assert_equal db, node.send("#{column_name}_in_database") end if child.nil? From 1d310894cd3b4554fa469b338234c0e0ed0ce4a8 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Tue, 28 Mar 2023 21:31:16 -0400 Subject: [PATCH 3/6] configure ancestry_column name from matrix Things changed a bit here. In tests, we are now always passing ancestry_column into has_ancestry (in with_model) This value is from an env variable or defaulting to ancestry So when code calls has_ancestry, we're not sure the name of the field in the database so we need to be more careful. Mostly noticeable in the tests that manually call has_ancestry And the test of the default ancestry column name (which assumed has_ancestry was being called without values) --- .github/workflows/run_test_suite.yml | 27 +++++++++++++++++++++------ test/concerns/build_ancestry_test.rb | 8 ++++++-- test/environment.rb | 12 ++++++++---- 3 files changed, 35 insertions(+), 12 deletions(-) diff --git a/.github/workflows/run_test_suite.yml b/.github/workflows/run_test_suite.yml index 0ae606e0..db313a95 100644 --- a/.github/workflows/run_test_suite.yml +++ b/.github/workflows/run_test_suite.yml @@ -68,6 +68,7 @@ jobs: MYSQL_PWD: password # for rails tests (from matrix) BUNDLE_GEMFILE: gemfiles/gemfile_${{ matrix.activerecord }}.gemfile + FORMAT: ${{ matrix.format }} steps: - name: checkout code uses: actions/checkout@v3 @@ -84,25 +85,39 @@ jobs: env: DB: sqlite3 run: | - FORMAT=${{ matrix.format }} bundle exec rake + bundle exec rake - name: run pg tests env: DB: pg run: | - FORMAT=${{ matrix.format }} bundle exec rake + bundle exec rake - name: run pg tests (UPDATE_STRATEGY=sql) env: DB: pg + UPDATE_STRATEGY: sql run: | - FORMAT=${{ matrix.format }} UPDATE_STRATEGY=sql bundle exec rake - + bundle exec rake + - name: run pg tests (ANCESTRY_COLUMN=ancestry_alt) + env: + DB: pg + ANCESTRY_COLUMN: ancestry_alt + run: | + bundle exec rake + - name: run pg tests (UPDATE_STRATEGY=sql ANCESTRY_COLUMN=ancestry_alt) + env: + DB: pg + ANCESTRY_COLUMN: ancestry_alt + UPDATE_STRATEGY: sql + run: | + bundle exec rake - name: run mysql tests env: DB: mysql2 run: | - FORMAT=${{ matrix.format }} bundle exec rake + bundle exec rake - name: run mysql tests (ANCESTRY_COLUMN_TYPE=binary) env: DB: mysql2 + ANCESTRY_COLUMN_TYPE: binary run: | - FORMAT=${{ matrix.format }} ANCESTRY_COLUMN_TYPE=binary bundle exec rake + bundle exec rake diff --git a/test/concerns/build_ancestry_test.rb b/test/concerns/build_ancestry_test.rb index 2cea51a6..f1d14e81 100644 --- a/test/concerns/build_ancestry_test.rb +++ b/test/concerns/build_ancestry_test.rb @@ -2,6 +2,8 @@ class BuildAncestryTest < ActiveSupport::TestCase def test_build_ancestry_from_parent_ids + ancestry_column = AncestryTestDatabase.ancestry_column + AncestryTestDatabase.with_model :skip_ancestry => true, :extra_columns => {:parent_id => :integer} do |model| [model.create!].each do |parent1| (Array.new(5) { model.create! :parent_id => parent1.id }).each do |parent2| @@ -14,7 +16,7 @@ def test_build_ancestry_from_parent_ids # Assert all nodes where created assert_equal (0..3).map { |n| 5 ** n }.sum, model.count - model.has_ancestry + model.has_ancestry ancestry_column: ancestry_column model.build_ancestry_from_parent_ids! # Assert ancestry integrity @@ -43,6 +45,8 @@ def test_build_ancestry_from_parent_ids end def test_build_ancestry_from_other_ids + ancestry_column = AncestryTestDatabase.ancestry_column + AncestryTestDatabase.with_model :skip_ancestry => true, :extra_columns => {:misc_id => :integer} do |model| [model.create!].each do |parent1| (Array.new(5) { model.create! :misc_id => parent1.id }).each do |parent2| @@ -55,7 +59,7 @@ def test_build_ancestry_from_other_ids # Assert all nodes where created assert_equal (0..3).map { |n| 5 ** n }.sum, model.count - model.has_ancestry + model.has_ancestry ancestry_column: ancestry_column model.build_ancestry_from_parent_ids! :misc_id # Assert ancestry integrity diff --git a/test/environment.rb b/test/environment.rb index e08609e7..c74ea478 100644 --- a/test/environment.rb +++ b/test/environment.rb @@ -56,7 +56,7 @@ def self.setup Ancestry.default_update_strategy = ENV["UPDATE_STRATEGY"] == "sql" ? :sql : :ruby Ancestry.default_ancestry_format = ENV["FORMAT"].to_sym if ENV["FORMAT"].present? - puts "testing #{db_type} #{Ancestry.default_update_strategy == :sql ? "(sql) " : ""}(with #{column_type} column)" + puts "testing #{db_type} #{Ancestry.default_update_strategy == :sql ? "(sql) " : ""}(with #{column_type} #{ancestry_column})" puts "column format: #{Ancestry.default_ancestry_format} options: #{column_options.inspect}" rescue => err @@ -71,7 +71,11 @@ def self.setup end def self.column_type - @column_type ||= ENV["COLUMN_TYPE"].presence || "string" + @column_type ||= ENV["ANCESTRY_COLUMN_TYPE"].presence || "string" + end + + def self.ancestry_column + @ancestry_column ||= ENV["ANCESTRY_COLUMN"].presence || "ancestry" end def self.ancestry_collation @@ -111,16 +115,16 @@ def self.column_options(force_allow_nil: false) def self.with_model options = {} depth = options.delete(:depth) || 0 width = options.delete(:width) || 0 - ancestry_column = options[:ancestry_column] || :ancestry skip_ancestry = options.delete(:skip_ancestry) extra_columns = options.delete(:extra_columns) default_scope_params = options.delete(:default_scope_params) + options[:ancestry_column] ||= ancestry_column table_options={} table_options[:id] = options.delete(:id) if options.key?(:id) ActiveRecord::Base.connection.create_table 'test_nodes', **table_options do |table| - table.send(column_type, ancestry_column, **column_options(force_allow_nil: skip_ancestry)) + table.send(column_type, options[:ancestry_column], **column_options(force_allow_nil: skip_ancestry)) table.integer options[:cache_depth] == true ? :ancestry_depth : options[:cache_depth] if options[:cache_depth] if options[:counter_cache] counter_cache_column = options[:counter_cache] == true ? :children_count : options[:counter_cache] From 92d9c6833bac29af94a801c84954e49716898e9b Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Tue, 28 Mar 2023 21:20:54 -0400 Subject: [PATCH 4/6] remove alt specific tests that are covered by other tests we are running all tests with an alt ancestry, so these are no longer necessary --- test/concerns/has_ancestry_test.rb | 24 ------------------------ test/concerns/sort_by_ancestry_test.rb | 13 ------------- 2 files changed, 37 deletions(-) diff --git a/test/concerns/has_ancestry_test.rb b/test/concerns/has_ancestry_test.rb index 81b7d772..01cc7e7f 100644 --- a/test/concerns/has_ancestry_test.rb +++ b/test/concerns/has_ancestry_test.rb @@ -61,18 +61,6 @@ def test_modified_parents_set_ancestry_properly end end - def test_set_parent_with_non_default_ancestry_column - AncestryTestDatabase.with_model :depth => 3, :width => 3, :ancestry_column => :alternative_ancestry do |_model, roots| - root1, root2, _root3 = roots.map(&:first) - assert_no_difference 'root1.descendants.size' do - assert_difference 'root2.descendants.size', root1.subtree.size do - root1.parent = root2 - root1.save! - end - end - end - end - def test_set_parent_id AncestryTestDatabase.with_model :depth => 3, :width => 3 do |_model, roots| root1, root2, _root3 = roots.map(&:first) @@ -85,18 +73,6 @@ def test_set_parent_id end end - def test_set_parent_id_with_non_default_ancestry_column - AncestryTestDatabase.with_model :depth => 3, :width => 3, :ancestry_column => :alternative_ancestry do |_model, roots| - root1, root2, _root3 = roots.map(&:first) - assert_no_difference 'root1.descendants.size' do - assert_difference 'root2.descendants.size', root1.subtree.size do - root1.parent_id = root2.id - root1.save! - end - end - end - end - def test_setup_test_nodes AncestryTestDatabase.with_model :depth => 3, :width => 3 do |model, roots| assert_equal Array, roots.class diff --git a/test/concerns/sort_by_ancestry_test.rb b/test/concerns/sort_by_ancestry_test.rb index be9c8692..768d2809 100644 --- a/test/concerns/sort_by_ancestry_test.rb +++ b/test/concerns/sort_by_ancestry_test.rb @@ -233,17 +233,4 @@ def test_sort_by_ancestry_with_block_paginated_missing_parents_and_children end end end - - def test_sort_by_ancestry_with_ancestry_column - AncestryTestDatabase.with_model :ancestry_column => :t1, :extra_columns => {:rank => :integer} do |model| - _, n2, n3, n4, n5, _ = build_ranked_tree(model) - - records = model.sort_by_ancestry(model.all.ordered_by_ancestry_and(:rank).offset(1).take(4), &RANK_SORT) - if CORRECT - assert_equal [n3, n2, n4, n5].map(&:id), records.map(&:id) - else - assert_equal [n2, n4, n5, n3].map(&:id), records.map(&:id) - end - end - end end From fd0b86d2156c99a55bf6473288023736b006f912 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Tue, 28 Mar 2023 21:39:17 -0400 Subject: [PATCH 5/6] version file bump --- CHANGELOG.md | 4 ++++ lib/ancestry/version.rb | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 278fb880..21e1af32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,10 @@ jobs. if you need to do this in the ui, use the depth_caching. * These are seen as internal and may go away: - `apply_orphan_strategy` (TODO: use `orphan_strategy => none` and define `before_destory`) +## Version [4.3.3] 2023-04-01 + +* Fix: sort_by_ancesty with custom ancestry_column [#656](https://github.com/stefankroes/ancestry/pull/656) (thx @mitsuru) + ## Version [4.3.2] 2023-03-25 * Fix: added back fields that were removed in #589 [#647](https://github.com/stefankroes/ancestry/pull/647) (thx @rastamhadi) diff --git a/lib/ancestry/version.rb b/lib/ancestry/version.rb index 20b245ea..08fbe39a 100644 --- a/lib/ancestry/version.rb +++ b/lib/ancestry/version.rb @@ -1,3 +1,3 @@ module Ancestry - VERSION = '4.3.1' + VERSION = '5.0.0' end From c4832d885a5753d8bed7e45c0c50b8bc4be7c68e Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Tue, 28 Mar 2023 22:06:11 -0400 Subject: [PATCH 6/6] moved duplicate testing code over to test_helpers --- test/concerns/materialized_path2_test.rb | 23 ----------------------- test/concerns/materialized_path_test.rb | 24 ------------------------ test/environment.rb | 3 ++- test/test_helpers.rb | 23 +++++++++++++++++++++++ 4 files changed, 25 insertions(+), 48 deletions(-) create mode 100644 test/test_helpers.rb diff --git a/test/concerns/materialized_path2_test.rb b/test/concerns/materialized_path2_test.rb index 2284d237..83a1ce98 100644 --- a/test/concerns/materialized_path2_test.rb +++ b/test/concerns/materialized_path2_test.rb @@ -97,27 +97,4 @@ def test_ancestry_validation_exclude_self end end end - - private - - def assert_ancestry(node, value, child: :skip, db: :value) - if value.nil? - assert_nil node.ancestry - else - assert_equal value, node.ancestry - end - - db = value if db == :value - if db.nil? - assert_nil node.ancestry_in_database - else - assert_equal db, node.ancestry_in_database - end - - if child.nil? - assert_nil node.child_ancestry - elsif child != :skip - assert_equal child, node.child_ancestry - end - end end diff --git a/test/concerns/materialized_path_test.rb b/test/concerns/materialized_path_test.rb index 4d38cdf1..328f7d12 100644 --- a/test/concerns/materialized_path_test.rb +++ b/test/concerns/materialized_path_test.rb @@ -99,28 +99,4 @@ def test_ancestry_validation_exclude_self end end end - - private - - def assert_ancestry(node, value, child: :skip, db: :value) - column_name = node.class.ancestry_column - if value.nil? - assert_nil node.send(column_name) - else - assert_equal value, node.send(column_name) - end - - db = value if db == :value - if db.nil? - assert_nil node.send("#{column_name}_in_database") - else - assert_equal db, node.send("#{column_name}_in_database") - end - - if child.nil? - assert_nil node.child_ancestry - elsif child != :skip - assert_equal child, node.child_ancestry - end - end end diff --git a/test/environment.rb b/test/environment.rb index c74ea478..8a486339 100644 --- a/test/environment.rb +++ b/test/environment.rb @@ -11,8 +11,9 @@ require 'active_support' require 'active_support/test_case' +require 'test_helpers' ActiveSupport.test_order = :random if ActiveSupport.respond_to?(:test_order=) - +ActiveSupport::TestCase.include(TestHelpers) require 'active_record' require 'logger' diff --git a/test/test_helpers.rb b/test/test_helpers.rb new file mode 100644 index 00000000..61caee1c --- /dev/null +++ b/test/test_helpers.rb @@ -0,0 +1,23 @@ +module TestHelpers + def assert_ancestry(node, value, child: :skip, db: :value) + column_name = node.class.ancestry_column + if value.nil? + assert_nil node.send(column_name) + else + assert_equal value, node.send(column_name) + end + + db = value if db == :value + if db.nil? + assert_nil node.send("#{column_name}_in_database") + else + assert_equal db, node.send("#{column_name}_in_database") + end + + if child.nil? + assert_nil node.child_ancestry + elsif child != :skip + assert_equal child, node.child_ancestry + end + end +end