Feature: Add Rails generator and rake task support for creating data migration test coverage by joshmfrankel · Pull Request #355 · ilyakatz/data-migrate

joshmfrankel · 2024-12-12T00:42:51Z

Why

Implements: #148

I recently needed well-factored test coverage for a complex data migration, so I thought I'd take an attempt at implementing test suite support for both RSpec and Minitest.

What

Adds new Config setting config.test_generator_enabled which conditionally creates associated tests files upon rails g data_migration add_this_to_that. (Default: false)
Adds basic support for inferring test suite based on available helper file
Adds new config setting config.test_generator_framework which can be set to :minitest or :rspec

Notes

I read the comment here: #162 (comment) regarding using Rails.configuration.generators.options[:rails]. That returns a very clear: Rails.configuration.generators.options[:rails][:test_framework] #=> :rspec. While testing Minitest and RSpec, I noticed that this comes from having the rspec-rails gem installed. It doesn't necessarily check for valid installation. I attempted a valid installation check by looking for the framework specific files test/test_helper.rb and spec/rails_helper.rb.

Resources

feature: Generate a spec file. #162

Also, kudos on a great gem! 💎

…ge on data migrations * New Rake task `rake data:tests:setup` * New Config setting `config.test_support = true/false` (false by default)

joshmfrankel · 2024-12-12T00:44:40Z

lib/data_migrate/helpers/infer_test_suite_type.rb

+  module Helpers
+    class InferTestSuiteType
+      def call
+        if File.exist?(Rails.root.join('spec', 'spec_helper.rb'))


We could perform add to the conditionals here for Rails.configuration.generators.options[:rails][:test_framework] == :rspec and Rails.configuration.generators.options[:rails][:test_framework] == :test_unit respectively to look for both installation and configuration.

I like the idea of getting the test framework from Rails's config. I've worked on projects where both rspec and minitest were used (migration happening but not over and very little bandwidth to finish it) and I think its easier for us to maintain compatibility with the configuration over the presence of files?

Perhaps adding a config line would work too, but I'd imagine projects running on sinatra for example to maybe not follow the conventions set elsewhere?

I added a simple configuration value to early return when set OR fallback to the inference logic.

joshmfrankel · 2024-12-12T00:46:45Z

lib/data_migrate/tasks/setup_tests.rb

+module DataMigrate
+  module Tasks
+    class SetupTests
+      INJECTION_MATCHER = Regexp.new(/require_relative ["|']\.\.\/config\/environment["|']/)


Had mixed feeling on this. Basically, I want to find a target phrase in either the rails_helper.rb or test_helper.rb. It seemed best to inject the require loop https://github.com/ilyakatz/data-migrate/pull/355/files#diff-ffd91a3de47aa2b12e05f99c726f74f27c7181b3f2f987c24aa9af32f72ea2acR57 after the environment was loaded. Open to other suggestions.

The setup task is also optional. You could just individually require files per data migration test file.

joshmfrankel · 2024-12-12T00:48:49Z

lib/data_migrate/tasks/setup_tests.rb

+      def test_helper_file_path
+        case DataMigrate::Helpers::InferTestSuiteType.new.call
+        when :rspec
+          Rails.root.join('spec', 'rails_helper.rb')


rails_helper.rb contains the injection target BUT DataMigrate::Helpers::InferTestSuiteType checks spec_helper.rb (see: https://github.com/ilyakatz/data-migrate/pull/355/files#diff-e3eb6de888eae80ac87faa33be0419ff8abdc2bd7d14e23c243304834d53604eR5). I'm thinking these maybe should match in their conditional checks. Thoughts?

I think if you were to remove the lines added to inject the require statement, this all disappears. It wouldn't be as full-fledged as this solution but avoids a bunch of complexity which users can just replace if they wished to do so.

A question that just came to mind, why not just inject the require statement inside the spec file generated?

A question that just came to mind, why not just inject the require statement inside the spec file generated?

That would definitely work for projects that have RSpec / Minitest configured with data_migrate at the start. My original intent here was to allow existing projects to run a Rake task to easily add the line to the spec file. Though to be honest I agree with your first statement regarding the complexity of all this to essentially inject two lines of code. I'm going to change this to simply be part of the readme and see if it cleans things up

vprigent · 2024-12-21T01:49:52Z

lib/data_migrate/tasks/setup_tests.rb

+      def lines_for_injection
+        [
+          "# data_migrate: Include data migrations for writing test coverage",
+          "Dir[Rails.root.join(DataMigrate.config.data_migrations_path, '*.rb')].each { |f| require f }"


https://github.com/vprigent/data-migrate/blob/517328051687873c9f8412efe2d6a8f2cf7e9157/README.md?plain=1#L167

DataMigrate.config.data_migrations_path can be an array of locations in order to support engine-based data migrations...

We could reverse this logic and do something like this perhaps:

Suggested change

"Dir[Rails.root.join(DataMigrate.config.data_migrations_path, '*.rb')].each { |f| require f }"

"Dir[*Array.wrap(DataMigrate.config.data_migrations_path).map{|path| Rails.root.join(path, '*.rb')}].each { |f| require f }"

I honestly think it would be easier to let the user require whichever file they are about to test within their spec file though?

I honestly think it would be easier to let the user require whichever file they are about to test within their spec file though?

Definitely true to explicitly include these. I'm thinking for ease-of-use, having an automated way to include any & all data files might make things easier than multiple require lines. Not all data migrations would be tested, though, so might be a good case for an explicit approach.

Good catch on engines also

vprigent · 2024-12-21T01:54:36Z

lib/data_migrate/tasks/setup_tests.rb

+      def test_helper_file_path
+        case DataMigrate::Helpers::InferTestSuiteType.new.call
+        when :rspec
+          Rails.root.join('spec', 'rails_helper.rb')


I think if you were to remove the lines added to inject the require statement, this all disappears. It wouldn't be as full-fledged as this solution but avoids a bunch of complexity which users can just replace if they wished to do so.

A question that just came to mind, why not just inject the require statement inside the spec file generated?

vprigent · 2024-12-21T01:55:48Z

lib/generators/data_migration/data_migration_generator.rb

+      end
+
+      def data_migrations_spec_file_path
+        File.join(Rails.root, 'spec', DataMigrate.config.data_migrations_path, "#{file_name}_spec.rb")


You may have issues here too with data_migrations_path not being necessarily a single path either here.

You could use the same logic here:
https://github.com/vprigent/data-migrate/blob/517328051687873c9f8412efe2d6a8f2cf7e9157/lib/generators/data_migration/data_migration_generator.rb#L61-L64

Though ideally we'd abstract that away and make a definitive location as being the location for new migrations while still allowing engines/libraries to integrate with data_migration in a way that's painless for users.

spec/generators/data_migration/data_migration_generator_spec.rb

…uire new data file in spec/test

joshmfrankel · 2025-09-11T15:28:04Z

lib/generators/data_migration/templates/data_migration_spec.rb

@@ -0,0 +1,10 @@
+require 'rails_helper'
+require './<%= data_migrations_file_path_with_version %>'


@vprigent Excellent idea here! Much easier and right next to the file being tested

joshmfrankel · 2025-09-11T15:29:36Z

Just recycled all proposed changes / suggestions. Much simpler implementation while preserving the ability to generate test files alongside migrations 🚀

Morozzzko

Nice work! I feel like we might merge it and start getting feedback on it!

Morozzzko · 2025-09-18T20:02:29Z

lib/data_migrate/config.rb

      @data_template_path = DEFAULT_DATA_TEMPLATE_PATH
      @db_configuration = nil
      @spec_name = nil
+      @test_support_enabled = false


Nitpick:

I've read your PR a few times so far. Decided to do one last round from the "does it make me confused if I jump straight into the code without reading the context?". So please consider my comments as ideas / suggestions, and not something authoritative.

In this instance, I feel like the name is a bit confusing. Because it's not that we turn on test support, but we turn on test generation. So I'd prefer to have test_generator_enabled, generate_tests, generate_specs, generate_test_files, etc.

I like this suggested change as it better represents what the configuration value achieves 💯

Morozzzko · 2025-09-18T20:06:55Z

lib/data_migrate/config.rb

      @db_configuration = nil
      @spec_name = nil
+      @test_support_enabled = false
+      @test_framework = nil


Nitpick:

It feels like the default shouldn't be nil, but we should attempt to infer rspec/minitest early.

I'm not sure it makes as much sense from the "config lifecycle" perspective, though. If not, I wonder if we can make the limitation a bit more obvious?

What if we were to push the InferTestSuiteType call up into the configuration initializer here? That way, inference check would only happen once (at configuration level), and we could move the early return out of the inference class entierly. Setting the test_framework would then override the inferred value for manual control over the generator.

Also, I'd like to update InferTestSuiteType => InferTestFramework to match the configuration value naming

# lib/data_migrate/config.rb def initialize @data_migrations_table_name = "data_migrations" @data_migrations_path = "db/data/" @data_template_path = DEFAULT_DATA_TEMPLATE_PATH @db_configuration = nil @spec_name = nil @test_support_enabled = false @test_framework = DataMigrate::Helpers::InferTestFramework.new.call end # lib/data_migrate/helpers/infer_test_framework.rb module DataMigrate module Helpers class InferTestFramework def call if File.exist?(Rails.root.join('spec', 'spec_helper.rb')) :rspec elsif File.exist?(Rails.root.join('test', 'test_helper.rb')) :minitest else raise StandardError.new('Unable to determine test suite') end end end end end # lib/generators/data_migration/data_migration_generator.rb def create_data_migration_test return unless DataMigrate.config.test_support_enabled case DataMigrate.config.test_framework when :rspec template "data_migration_spec.rb", data_migrations_spec_file_path when :minitest template "data_migration_test.rb", data_migrations_test_file_path end end

I believe this cleans up the logic and keeps a good separation of responsibility throughout the flow. Thoughts on the above?

Morozzzko · 2025-09-18T20:07:33Z

lib/generators/data_migration/data_migration_generator.rb

+      def create_data_migration_test
+        return unless DataMigrate.config.test_support_enabled
+
+        case DataMigrate::Helpers::InferTestSuiteType.new.call


nitpick:

... adding to my previous comment: would be nice if we could always rely on config here, instead of manually infering things each time

* Update name of configuration value test_support_enabled => test_generator_enabled * Update name of configuration value test_framework => test_generator_framework * Config now automatically sets the test_generator_framework by inference and allows for overriding * Removed file system calls to Rails in favor of File.expand_path * Utilize configuration value to determine which generated spec/test file to create

joshmfrankel · 2025-09-29T13:53:34Z

I've made several updates in the latest commit: b74b7f9

joshmfrankel · 2025-10-02T18:57:52Z

spec/generators/data_migration/data_migration_generator_spec.rb

+      end
+
+      after(:all) do
+        FileUtils.rm_rf(Dir.pwd + '/db/data')


Fun fact: Not setting DataMigrate.config.data_migrations_path and trying to use it to cleanup test files directories, results in the spec trying to cleanup your root filesystem /. 🔥 I had the fun time of finding that out the hard way. The above is much safer now, also likely why all the tests were failing

Feature: Add generator and rake task support for creating test covera…

5173280

…ge on data migrations * New Rake task `rake data:tests:setup` * New Config setting `config.test_support = true/false` (false by default)

joshmfrankel commented Dec 12, 2024

View reviewed changes

joshmfrankel mentioned this pull request Dec 10, 2024

Well-factored data migrations joshmfrankel/joshmfrankel.github.io#88

Closed

joshmfrankel marked this pull request as ready for review December 12, 2024 18:08

vprigent reviewed Dec 21, 2024

View reviewed changes

vprigent reviewed Dec 25, 2024

View reviewed changes

spec/generators/data_migration/data_migration_generator_spec.rb Show resolved Hide resolved

joshmfrankel added 3 commits September 11, 2025 11:23

Remove SetupTests rake task to simplify configuration

f14dbf8

Add config.test_framework, Add support for template to explicitly req…

6b9cdee

…uire new data file in spec/test

Merge branch 'main' into main

7cfcc27

joshmfrankel commented Sep 11, 2025

View reviewed changes

joshmfrankel requested a review from vprigent September 11, 2025 15:28

Morozzzko approved these changes Sep 18, 2025

View reviewed changes

Fix dangerous cleanup callback

02b4b12

joshmfrankel commented Oct 2, 2025

View reviewed changes

joshmfrankel requested a review from Morozzzko October 2, 2025 19:01

joshmfrankel added 2 commits October 6, 2025 12:44

Make those comments pretty

45e97b1

Merge branch 'main' into main

3fb736a

	"Dir[Rails.root.join(DataMigrate.config.data_migrations_path, '*.rb')].each { \|f\| require f }"
	"Dir[Array.wrap(DataMigrate.config.data_migrations_path).map{\|path\| Rails.root.join(path, '.rb')}].each { \|f\| require f }"

		@@ -0,0 +1,10 @@
		require 'rails_helper'
		require './<%= data_migrations_file_path_with_version %>'

Conversation

joshmfrankel commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What

Notes

Resources

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshmfrankel Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshmfrankel Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshmfrankel commented Sep 11, 2025

Uh oh!

Morozzzko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshmfrankel Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshmfrankel commented Sep 29, 2025

Uh oh!

joshmfrankel Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joshmfrankel commented Dec 12, 2024 •

edited

Loading

joshmfrankel Sep 11, 2025 •

edited

Loading

joshmfrankel Sep 11, 2025 •

edited

Loading

joshmfrankel Sep 19, 2025 •

edited

Loading

joshmfrankel Oct 2, 2025 •

edited

Loading