From 67e9f7e0fcfe2d810a972f043da325cb76133f87 Mon Sep 17 00:00:00 2001 From: Jesse McGinnis Date: Mon, 6 May 2024 19:28:42 -0400 Subject: [PATCH] Add FactoryBot and round out unit tests --- Gemfile | 3 +- Gemfile.lock | 6 + app/models/property_value.rb | 6 +- test/factories/category_factory.rb | 14 +++ test/factories/property_factory.rb | 10 ++ test/factories/property_value_factory.rb | 10 ++ test/test_helper.rb | 19 +++- test/unit/application_test.rb | 6 +- test/unit/category_test.rb | 103 +++++++++--------- test/unit/property_test.rb | 80 ++++++++++++++ test/unit/property_value_test.rb | 69 ++++++------ .../categories_property_serializer_test.rb | 2 +- .../source/category_serializer_test.rb | 36 +++--- 13 files changed, 251 insertions(+), 113 deletions(-) create mode 100644 test/factories/category_factory.rb create mode 100644 test/factories/property_factory.rb create mode 100644 test/factories/property_value_factory.rb create mode 100644 test/unit/property_test.rb diff --git a/Gemfile b/Gemfile index cb5fe5ce1..04587dbff 100644 --- a/Gemfile +++ b/Gemfile @@ -3,16 +3,17 @@ source "https://rubygems.org" gem "activerecord", "~> 7.1" +gem "bundler" gem "jekyll", "~> 4.3" gem "rake", "~> 13.1" gem "sqlite3", "~> 1.7" gem "zeitwerk", "~> 2.6" gem "byebug" -gem "bundler" gem "rubocop-shopify", require: false group :test do + gem "factory_bot_rails", "~> 6.4" gem "minitest", "~> 5.21" gem "minitest-rails", "~> 7.1" gem "minitest-hooks", "~> 1.5" diff --git a/Gemfile.lock b/Gemfile.lock index de16d070a..cc0cf381d 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -50,6 +50,11 @@ GEM http_parser.rb (~> 0) erubi (1.12.0) eventmachine (1.2.7) + factory_bot (6.4.6) + activesupport (>= 5.0.0) + factory_bot_rails (6.4.3) + factory_bot (~> 6.4) + railties (>= 5.0.0) ffi (1.16.3) forwardable-extended (2.6.0) google-protobuf (4.26.1-arm64-darwin) @@ -195,6 +200,7 @@ DEPENDENCIES activerecord (~> 7.1) bundler byebug + factory_bot_rails (~> 6.4) jekyll (~> 4.3) minitest (~> 5.21) minitest-hooks (~> 1.5) diff --git a/app/models/property_value.rb b/app/models/property_value.rb index 313ed0161..eb46561e2 100644 --- a/app/models/property_value.rb +++ b/app/models/property_value.rb @@ -15,12 +15,12 @@ class PropertyValue < ApplicationRecord foreign_key: :primary_property_friendly_id, primary_key: :friendly_id - def primary_property_friendly_id=(id) - self.primary_property = Property.find_by(friendly_id: id) + def primary_property_friendly_id=(friendly_id) + self.primary_property = Property.find_by(friendly_id:) end validates :name, presence: true - validates :friendly_id, presence: true + validates :friendly_id, presence: true, uniqueness: true validates :handle, presence: true, uniqueness: { scope: :primary_property_friendly_id } def gid diff --git a/test/factories/category_factory.rb b/test/factories/category_factory.rb new file mode 100644 index 000000000..0d2d0227b --- /dev/null +++ b/test/factories/category_factory.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :category do + sequence(:id, 1) do + if parent.nil? + "aa" + else + "#{parent.id}-#{_1}" + end + end + name { "Category #{id}" } + end +end diff --git a/test/factories/property_factory.rb b/test/factories/property_factory.rb new file mode 100644 index 000000000..ea4a5b840 --- /dev/null +++ b/test/factories/property_factory.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :property do + sequence(:id, 1) + name { "Property#{id}" } + handle { name.downcase } + friendly_id { handle } + end +end diff --git a/test/factories/property_value_factory.rb b/test/factories/property_value_factory.rb new file mode 100644 index 000000000..a5a968d7a --- /dev/null +++ b/test/factories/property_value_factory.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :property_value do + sequence(:id, 1) + name { "Value#{id}" } + handle { name.downcase } + friendly_id { [primary_property&.handle, handle].compact.join("__") } + end +end diff --git a/test/test_helper.rb b/test/test_helper.rb index 517bc90a5..e2f4cc209 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -1,11 +1,24 @@ # frozen_string_literal: true require "bundler/setup" - Bundler.require(:test) + +require "minitest/rails" +require "minitest/pride" require_relative "../application" + Application.establish_db_connection!(env: :test) Application.load_and_reset_schema! -require "minitest/rails" -require "minitest/pride" +FactoryBot.find_definitions + +class ApplicationTestCase < ActiveSupport::TestCase + include FactoryBot::Syntax::Methods + include Minitest::Hooks + + def around_all + ActiveRecord::Base.transaction do + super + end + end +end diff --git a/test/unit/application_test.rb b/test/unit/application_test.rb index 267d5af11..6c78e5524 100644 --- a/test/unit/application_test.rb +++ b/test/unit/application_test.rb @@ -2,7 +2,7 @@ require_relative "../test_helper" -class ApplicationTest < ActiveSupport::TestCase +class ApplicationTest < ApplicationTestCase test "Zeitwerk compliance" do LOADER.eager_load(force: true) rescue Zeitwerk::NameError => e @@ -10,4 +10,8 @@ class ApplicationTest < ActiveSupport::TestCase else assert(true) end + + test "Factories are valid" do + FactoryBot.lint(traits: true) + end end diff --git a/test/unit/category_test.rb b/test/unit/category_test.rb index 20c76d854..585dda4b4 100644 --- a/test/unit/category_test.rb +++ b/test/unit/category_test.rb @@ -2,102 +2,99 @@ require_relative "../test_helper" -class CategoryTest < ActiveSupport::TestCase +class CategoryTest < ApplicationTestCase def teardown - Category.destroy_all + Category.delete_all end test ".gid returns a global id" do - assert_equal "gid://shopify/TaxonomyCategory/tt", Category.gid("tt") + assert_equal "gid://shopify/TaxonomyCategory/aa", Category.gid("aa") end test ".parent_id_of returns the parent ID" do - assert_nil Category.parent_id_of("tt") - assert_equal "tt", Category.parent_id_of("tt-0") - assert_equal "tt-0", Category.parent_id_of("tt-0-1") + assert_nil Category.parent_id_of("aa") + assert_equal "aa", Category.parent_id_of("aa-0") + assert_equal "aa-0", Category.parent_id_of("aa-0-1") end - test "#id must follow parent" do - assert_predicate Category.new(id: "tt", name: "Root"), :valid? - assert_predicate Category.new(id: "tt-0", name: "Child", parent: category), :valid? - assert_predicate Category.new(id: "tt-123232", name: "Big child", parent: category), :valid? + test "#id must follow parent's id" do + assert_predicate build(:category, id: "aa-0", parent:), :valid? + assert_predicate build(:category, id: "aa-123232", parent:), :valid? + assert_predicate build(:category, id: "bb-0", parent:), :invalid? - assert_predicate Category.new(id: "aa-0", name: "Child", parent: category), :invalid? + child = build(:category, id: "aa-0", parent:) + assert_predicate build(:category, id: "aa-0-1", parent: child), :valid? + assert_predicate build(:category, id: "aa-1-1", parent: child), :invalid? + end - child = Category.new(id: "tt-0", name: "Child", parent: category) - assert_predicate Category.new(id: "tt-1-1", name: "Child", parent: child), :invalid? + test "#id for root must be 2 chars" do + assert_predicate build(:category, id: "t"), :invalid? + assert_predicate build(:category, id: "ttt"), :invalid? + assert_predicate build(:category, id: "01"), :invalid? end - test "#id must follow a strict format" do - assert_predicate Category.new(id: "t", name: "Root too short"), :invalid? - assert_predicate Category.new(id: "ttt", name: "Root too long"), :invalid? - assert_predicate Category.new(id: "01", name: "Root of numbers"), :invalid? - assert_predicate Category.new(id: "tt-t", name: "Child of letters", parent: category), :invalid? + test "#id for roots must not have dashes" do + assert_predicate build(:category, id: "aa-t"), :invalid? end test "#id must match depth" do - assert_predicate Category.new(id: "tt-0", name: "Child", parent: category), :valid? - - assert_predicate Category.new(id: "tt-0-1", name: "Child with grandchild ID", parent: category), :invalid? + assert_predicate build(:category, id: "aa-0", parent:), :valid? + assert_predicate build(:category, id: "aa-0-1", parent:), :invalid? end test "#gid returns a global id" do - assert_equal "gid://shopify/TaxonomyCategory/tt", category.gid + assert_equal "gid://shopify/TaxonomyCategory/aa", parent.gid + assert_equal "gid://shopify/TaxonomyCategory/aa-42", build(:category, id: "aa-42").gid end test "#root returns the top-most category node" do - child_category = Category.new(id: "tt-6", name: "Child", parent: category) - grandchild_category = Category.new(id: "tt-6-1", name: "Grandchild", parent: child_category) - - assert_equal category, child_category.root - assert_equal category, grandchild_category.root + assert_equal parent, child.root + assert_equal parent, grandchild.root end test "#ancestors walk up the tree" do - child_category = Category.new(id: "tt-7", name: "Child", parent: category) - grandchild_category = Category.new(id: "tt-7-1", name: "Grandchild", parent: child_category) - category.reload - - assert_equal [child_category, category], grandchild_category.ancestors + assert_equal [child, parent], grandchild.ancestors end test "#ancestors_and_self includes self" do - child_category = Category.create(id: "tt-7", name: "Child", parent: category) - grandchild_category = Category.new(id: "tt-7-1", name: "Grandchild", parent: child_category) - category.reload - - assert_equal [grandchild_category, child_category, category], grandchild_category.ancestors_and_self + assert_equal [grandchild, child, parent], grandchild.ancestors_and_self end test "#children are sorted by name" do - beta_child = Category.create(id: "tt-1", name: "Beta", parent: category) - alpha_child = Category.create(id: "tt-2", name: "Alpha", parent: category) - category.reload + beta_child = create(:category, name: "Beta", parent:) + alpha_child = create(:category, name: "Alpha", parent:) + parent.reload - assert_equal [alpha_child, beta_child], category.children.to_a + assert_equal [alpha_child, beta_child], parent.children.to_a end test "#descendants is depth-first" do - l2_category = Category.create(id: "tt-8", name: "Child", parent: category) - l3_category = Category.create(id: "tt-8-1", name: "Grandchild", parent: l2_category) - l3_sibling = Category.create(id: "tt-8-2", name: "Alpha Grandchild", parent: l2_category) - l4_category = Category.create(id: "tt-8-2-1", name: "Great Grandchild", parent: l3_sibling) - category.reload + l2_beta = create(:category, name: "Beta", parent: child) + l2_alpha = create(:category, name: "Alpha", parent: child) + l3_child = create(:category, parent: l2_alpha) + parent.reload - assert_equal [l2_category, l3_sibling, l4_category, l3_category], category.descendants + assert_equal [child, l2_alpha, l3_child, l2_beta], parent.descendants end test "#descendants_and_self includes self" do - child_category = Category.create(id: "tt-7", name: "Child", parent: category) - grandchild_category = Category.create(id: "tt-7-1", name: "Grandchild", parent: child_category) - category.reload + grandchild.save! + parent.reload - assert_equal [category, child_category, grandchild_category], category.descendants_and_self + assert_equal [parent, child, grandchild], parent.descendants_and_self end private - def category - @category ||= Category.create!(id: "tt", name: "Electronics") + def parent + @parent ||= build(:category, id: "aa") + end + + def child + @child ||= build(:category, parent:) + end + + def grandchild + @grandchild ||= build(:category, parent: child) end end diff --git a/test/unit/property_test.rb b/test/unit/property_test.rb new file mode 100644 index 000000000..e4962a8c1 --- /dev/null +++ b/test/unit/property_test.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +require_relative "../test_helper" + +class PropertyTest < ApplicationTestCase + def teardown + Property.delete_all + end + + test "default ordering is alphabetical" do + material = create(:property, name: "Material") + size = create(:property, name: "size") + color = create(:property, name: "Color") + + assert_equal [color, material, size], Property.all.to_a + end + + test ".base returns base properties" do + base_property.save! + extended_property.save! + + assert_equal [base_property], Property.base + end + + test ".extended returns properties based off others" do + base_property.save! + extended_property.save! + + assert_equal [extended_property], Property.extended + end + + test "#gid returns a global id" do + assert_equal "gid://shopify/TaxonomyAttribute/42", build(:property, id: 42).gid + end + + test "#gid returns base_property.gid when extended" do + refute_equal base_property.id, extended_property.id + assert_equal base_property.gid, extended_property.gid + end + + test "#base?" do + assert_predicate base_property, :base? + refute_predicate extended_property, :base? + end + + test "#extended?" do + refute_predicate base_property, :extended? + assert_predicate extended_property, :extended? + end + + test "#friendly_id must be unique" do + create(:property, friendly_id: "material") + another_material = build(:property, friendly_id: "material") + + refute_predicate another_material, :valid? + end + + test "#property_values must match base_property#property_values" do + value = build(:property_value) + base_property.property_values = [value] + extended_property.property_values = [value] + + assert_predicate base_property, :valid? + assert_predicate extended_property, :valid? + + extended_property.property_values = [] + + refute_predicate extended_property, :valid? + end + + private + + def base_property + @base_property ||= build(:property) + end + + def extended_property + @extended_property ||= build(:property, base_property:) + end +end diff --git a/test/unit/property_value_test.rb b/test/unit/property_value_test.rb index 4e05d33f5..dc4be3876 100644 --- a/test/unit/property_value_test.rb +++ b/test/unit/property_value_test.rb @@ -2,63 +2,56 @@ require_relative "../test_helper" -class PropertyValueTest < ActiveSupport::TestCase +class PropertyValueTest < ApplicationTestCase def teardown - Property.destroy_all - PropertyValue.destroy_all + Property.delete_all + PropertyValue.delete_all end - test "#gid returns a global id" do - assert_equal "gid://shopify/TaxonomyValue/12", value(id: 12).gid - end - - test "default ordering is alphabetical with 'other' last" do - red = value!(id: 11, name: "Red", friendly_id: "test__red", handle: "red") - zoo = value!(id: 12, name: "Zoo", friendly_id: "test__zoo", handle: "zoo") - other = value!(id: 10, name: "Other", friendly_id: "test__other", handle: "other") + test "default ordering is alphabetical with 'Other' last" do + other = create(:property_value, name: "Other") + zoo = create(:property_value, name: "Zoo") + red = create(:property_value, name: "Red") assert_equal [red, zoo, other], PropertyValue.all.to_a end - test "#full_name returns the name of the primary property and the value" do - color = Property.create!(name: "Color", friendly_id: "color", handle: "color") - red = value!(name: "Red", friendly_id: "color__red", primary_property: color) + test "#gid returns a global id" do + assert_equal "gid://shopify/TaxonomyValue/42", build(:property_value, id: 42).gid + end - assert_equal "Red [Color]", red.full_name + test "#full_name returns the name of the primary property and the value" do + assert_equal "Gold [Color]", build(:property_value, name: "Gold", primary_property: color_property).full_name end test "#full_name is just name if primary property is missing" do - red = value!(name: "Red", friendly_id: "color__red", handle: "red") - - assert_equal "Red", red.full_name + assert_equal "Foo", build(:property_value, name: "Foo").full_name end - test "#handle must be unique per primary friendly id" do - Property.create!(name: "Bar", friendly_id: "bar", handle: "bar") - value!(handle: "foo", primary_property_friendly_id: "bar") - assert_raises(ActiveRecord::RecordInvalid) { value!(id: 2, handle: "foo", primary_property_friendly_id: "bar") } + test "#friendly_id must be unique" do + create(:property_value, friendly_id: "gold") + another_gold = build(:property_value, friendly_id: "gold") + + refute_predicate another_gold, :valid? end - test "handle can be duplicated across different primary property friendly ids" do - Property.create!(name: "Bat", friendly_id: "bat", handle: "bat") - Property.create!(name: "Baz", friendly_id: "baz", handle: "baz") - value!(handle: "foo", primary_property_friendly_id: "bar") - value!(id: 2, friendly_id: "test_boo", handle: "foo", primary_property_friendly_id: "baz") + test "#handle must be unique per primary property" do + create(:property_value, handle: "gold", primary_property: color_property) + another_gold = build(:property_value, handle: "gold", primary_property: color_property) + + refute_predicate another_gold, :valid? end - private + test "#handle can be duplicated across different primary properties" do + create(:property_value, handle: "gold", primary_property: color_property) + material_gold = build(:property_value, handle: "gold", primary_property: build(:property, name: "Material")) - def value(**args) - default_args = { - id: 1, - name: "Foo", - friendly_id: "test__foo", - handle: "foo", - } - PropertyValue.new(default_args.merge(args)) + assert_predicate material_gold, :valid? end - def value!(**args) - value(**args).tap(&:save!) + private + + def color_property + @color_property ||= build(:property, name: "Color") end end diff --git a/test/unit/serializers/source/categories_property_serializer_test.rb b/test/unit/serializers/source/categories_property_serializer_test.rb index 2651dc811..dc4d347c8 100644 --- a/test/unit/serializers/source/categories_property_serializer_test.rb +++ b/test/unit/serializers/source/categories_property_serializer_test.rb @@ -4,7 +4,7 @@ module Serializers module Source - class CategoriesPropertySerializerTest < ActiveSupport::TestCase + class CategoriesPropertySerializerTest < ApplicationTestCase test "#unpack is an array of joins" do data = { "id" => 1, diff --git a/test/unit/serializers/source/category_serializer_test.rb b/test/unit/serializers/source/category_serializer_test.rb index 2bf01b10a..5c9839546 100644 --- a/test/unit/serializers/source/category_serializer_test.rb +++ b/test/unit/serializers/source/category_serializer_test.rb @@ -4,7 +4,12 @@ module Serializers module Source - class CategorySerializerTest < ActiveSupport::TestCase + class CategorySerializerTest < ApplicationTestCase + def teardown + Category.delete_all + Property.delete_all + end + test "#unpack is well formed" do data = { "id" => "aa-123", @@ -51,7 +56,7 @@ class CategorySerializerTest < ActiveSupport::TestCase end test "#pack is well formed for simple Category" do - category = Category.new(id: "aa-123", name: "foo") + category = build(:category, id: "aa-123", name: "foo") assert_equal( { @@ -65,24 +70,29 @@ class CategorySerializerTest < ActiveSupport::TestCase end test "#pack_all is well formed for multiple Categories" do - categories = [ - Category.new(id: "aa-123", name: "foo"), - Category.new(id: "bb", name: "bar"), - ] + Category.delete_all + + color = build(:property, name: "Color") + size = build(:property, name: "Size") + parent = build(:category, id: "aa", properties: [color]) + child = create(:category, id: "aa-123", parent:, properties: [color, size]) + + parent.reload + categories = [parent, child] assert_equal( [ { - "id" => "aa-123", - "name" => "foo", - "children" => [], - "attributes" => [], + "id" => "aa", + "name" => "Category aa", + "children" => ["aa-123"], + "attributes" => ["color"], }, { - "id" => "bb", - "name" => "bar", + "id" => "aa-123", + "name" => "Category aa-123", "children" => [], - "attributes" => [], + "attributes" => ["color", "size"], }, ], ::Source::CategorySerializer.pack_all(categories),