-
Notifications
You must be signed in to change notification settings - Fork 198
Feature: Add Rails generator and rake task support for creating data migration test coverage #355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
5173280
f14dbf8
6b9cdee
7cfcc27
b74b7f9
02b4b12
45e97b1
3fb736a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| module DataMigrate | ||
| module Helpers | ||
| class InferTestSuiteType | ||
| 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 | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,76 @@ | ||||||
| # frozen_string_literal: true | ||||||
|
|
||||||
| module DataMigrate | ||||||
| module Tasks | ||||||
| class SetupTests | ||||||
| INJECTION_MATCHER = Regexp.new(/require_relative ["|']\.\.\/config\/environment["|']/) | ||||||
|
||||||
|
|
||||||
| def call | ||||||
| return if injection_exists? | ||||||
|
|
||||||
| if find_injection_location.nil? | ||||||
| puts 'data_migrate: config/environment.rb was not found in the test helper file.' | ||||||
| return | ||||||
| end | ||||||
|
|
||||||
| add_newline | ||||||
|
|
||||||
| lines_for_injection.reverse.each do |line| | ||||||
| file_contents.insert(find_injection_location, "#{line}\n") | ||||||
| end | ||||||
|
|
||||||
| add_newline | ||||||
|
|
||||||
| File.open(test_helper_file_path, 'w') do |file| | ||||||
| file.puts file_contents | ||||||
| end | ||||||
|
|
||||||
| puts 'data_migrate: Test setup complete.' | ||||||
| end | ||||||
|
|
||||||
| private | ||||||
|
|
||||||
| def test_helper_file_path | ||||||
| case DataMigrate::Helpers::InferTestSuiteType.new.call | ||||||
| when :rspec | ||||||
| Rails.root.join('spec', 'rails_helper.rb') | ||||||
|
||||||
| when :minitest | ||||||
| Rails.root.join('test', 'test_helper.rb') | ||||||
| end | ||||||
| end | ||||||
|
|
||||||
| def file_contents | ||||||
| @_file_contents ||= File.readlines(test_helper_file_path) | ||||||
| end | ||||||
|
|
||||||
| def find_injection_location | ||||||
| @_find_injection_location ||= begin | ||||||
| index = file_contents.index { |line| line.match?(INJECTION_MATCHER) } | ||||||
| index.present? ? index + 1 : nil | ||||||
| end | ||||||
| end | ||||||
|
|
||||||
| def add_newline | ||||||
| file_contents.insert(find_injection_location, "\n") | ||||||
| end | ||||||
|
|
||||||
| 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 }" | ||||||
|
||||||
| "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 }" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly think it would be easier to let the user require whichever file they are about to test within their spec file though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,10 +15,30 @@ class DataMigrationGenerator < Rails::Generators::NamedBase | |
| def create_data_migration | ||
| set_local_assigns! | ||
| migration_template template_path, data_migrations_file_path | ||
| create_data_migration_test | ||
| end | ||
|
|
||
| protected | ||
|
|
||
| def create_data_migration_test | ||
| return unless DataMigrate.config.test_support_enabled | ||
|
|
||
| case DataMigrate::Helpers::InferTestSuiteType.new.call | ||
|
||
| 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 | ||
|
|
||
| def data_migrations_test_file_path | ||
| File.join(Rails.root, 'test', DataMigrate.config.data_migrations_path, "#{file_name}_test.rb") | ||
| end | ||
|
|
||
| def data_migrations_spec_file_path | ||
| File.join(Rails.root, 'spec', DataMigrate.config.data_migrations_path, "#{file_name}_spec.rb") | ||
|
||
| end | ||
|
|
||
| def set_local_assigns! | ||
| if file_name =~ /^(add|remove)_.*_(?:to|from)_(.*)/ | ||
| @migration_action = $1 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| require 'rails_helper' | ||
|
|
||
| describe <%= migration_class_name %>, type: :data_migration do | ||
| let(:migration) { <%= migration_class_name %>.new } | ||
|
|
||
| pending "should test `migration.up`" | ||
|
|
||
| pending "should test `migration.down`" | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| require 'test_helper' | ||
|
|
||
| class <%= migration_class_name %>Test < ActiveSupport::TestCase | ||
| def setup | ||
| @migration = <%= migration_class_name %>.new | ||
| end | ||
|
|
||
| def test_migration_up | ||
| skip("Pending test coverage for @migration.up") | ||
| end | ||
|
|
||
| def test_migration_down | ||
| skip("Pending test coverage for @migration.down") | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| require 'spec_helper' | ||
|
|
||
| describe DataMigrate::Helpers::InferTestSuiteType do | ||
| subject(:infer_test_suite) { described_class.new } | ||
|
|
||
| describe '#call' do | ||
| before do | ||
| allow(Rails).to receive(:root).and_return(Pathname.new('/fake/path')) | ||
| end | ||
|
|
||
| context 'when RSpec is detected' do | ||
| before do | ||
| allow(File).to receive(:exist?).with(Rails.root.join('spec', 'spec_helper.rb')).and_return(true) | ||
| allow(File).to receive(:exist?).with(Rails.root.join('test', 'test_helper.rb')).and_return(false) | ||
| end | ||
|
|
||
| it 'returns :rspec' do | ||
| expect(infer_test_suite.call).to eq(:rspec) | ||
| end | ||
| end | ||
|
|
||
| context 'when Minitest is detected' do | ||
| before do | ||
| allow(File).to receive(:exist?).with(Rails.root.join('spec', 'spec_helper.rb')).and_return(false) | ||
| allow(File).to receive(:exist?).with(Rails.root.join('test', 'test_helper.rb')).and_return(true) | ||
| end | ||
|
|
||
| it 'returns :minitest' do | ||
| expect(infer_test_suite.call).to eq(:minitest) | ||
| end | ||
| end | ||
|
|
||
| context 'when no test suite is detected' do | ||
| before do | ||
| allow(File).to receive(:exist?).with(Rails.root.join('spec', 'spec_helper.rb')).and_return(false) | ||
| allow(File).to receive(:exist?).with(Rails.root.join('test', 'test_helper.rb')).and_return(false) | ||
| end | ||
|
|
||
| it 'raises an error' do | ||
| expect { infer_test_suite.call }.to raise_error(StandardError, 'Unable to determine test suite') | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require "spec_helper" | ||
|
|
||
| describe DataMigrate::Tasks::SetupTests do | ||
| let(:file_contents_with_injection) do | ||
| <<~FILE_CONTENTS | ||
| # This file is copied to spec/ when you run 'rails generate rspec:install' | ||
| require 'spec_helper' | ||
| ENV['RAILS_ENV'] ||= 'test' | ||
| require_relative '../config/environment' | ||
|
|
||
| # data_migrate: Include data migrations for writing test coverage | ||
| Dir[Rails.root.join(DataMigrate.config.data_migrations_path, '*.rb')].each { |f| require f } | ||
| FILE_CONTENTS | ||
| end | ||
| let(:file_contents_without_injection) do | ||
| <<~FILE_CONTENTS | ||
| # This file is copied to spec/ when you run 'rails generate rspec:install' | ||
| require 'spec_helper' | ||
| ENV['RAILS_ENV'] ||= 'test' | ||
| require_relative '../config/environment' | ||
| FILE_CONTENTS | ||
| end | ||
| let(:file_contents_without_injection_matcher) do | ||
| <<~FILE_CONTENTS | ||
| # This file is copied to spec/ when you run 'rails generate rspec:install' | ||
| require 'spec_helper' | ||
| ENV['RAILS_ENV'] ||= 'test' | ||
| FILE_CONTENTS | ||
| end | ||
| let(:rails_root) { Pathname.new('/fake/app') } | ||
| let(:test_suite_inferrer) { instance_double(DataMigrate::Helpers::InferTestSuiteType) } | ||
|
|
||
| before do | ||
| allow(Rails).to receive(:root).and_return(rails_root) | ||
| allow(DataMigrate::Helpers::InferTestSuiteType).to receive(:new).and_return(test_suite_inferrer) | ||
| end | ||
|
|
||
| describe "#call" do | ||
| context 'when the injected code already exists' do | ||
| it 'returns early' do | ||
| allow(test_suite_inferrer).to receive(:call).and_return(:rspec) | ||
| allow(File).to receive(:readlines).and_return(file_contents_with_injection.lines) | ||
|
|
||
| expect(File).not_to receive(:open) | ||
|
|
||
| expect { | ||
| DataMigrate::Tasks::SetupTests.new.call | ||
| }.to output(/data_migrate: Test setup already exists./).to_stdout | ||
| end | ||
|
|
||
| context 'when the INJECTION_MATCHER is not found' do | ||
| it 'returns early' do | ||
| allow(test_suite_inferrer).to receive(:call).and_return(:rspec) | ||
| allow(File).to receive(:readlines).and_return(file_contents_without_injection_matcher.lines) | ||
|
|
||
| expect(File).not_to receive(:open) | ||
|
|
||
| expect { | ||
| DataMigrate::Tasks::SetupTests.new.call | ||
| }.to output(/data_migrate: config\/environment.rb was not found in the test helper file./).to_stdout | ||
| end | ||
| end | ||
|
|
||
| context 'for RSpec' do | ||
| it 'calls File.open for writing to rails_helper.rb' do | ||
| allow(test_suite_inferrer).to receive(:call).and_return(:rspec) | ||
| allow(File).to receive(:readlines).and_return(file_contents_without_injection.lines) | ||
|
|
||
| expect(File).to receive(:open).with(rails_root.join('spec', 'rails_helper.rb'), 'w') | ||
|
|
||
| expect { | ||
| DataMigrate::Tasks::SetupTests.new.call | ||
| }.to output(/data_migrate: Test setup complete./).to_stdout | ||
| end | ||
| end | ||
|
|
||
| context 'for Minitest' do | ||
| it 'calls File.open for writing to test_helper.rb' do | ||
| allow(test_suite_inferrer).to receive(:call).and_return(:minitest) | ||
| allow(File).to receive(:readlines).and_return(file_contents_without_injection.lines) | ||
|
|
||
| expect(File).to receive(:open).with(rails_root.join('test', 'test_helper.rb'), 'w') | ||
|
|
||
| expect { | ||
| DataMigrate::Tasks::SetupTests.new.call | ||
| }.to output(/data_migrate: Test setup complete./).to_stdout | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this suggested change as it better represents what the configuration value achieves 💯