Skip to content
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

Add a test requiring data migrations to have an associated spec file #247

Merged
merged 1 commit into from
Aug 9, 2018

Conversation

bdunne
Copy link
Member

@bdunne bdunne commented Aug 8, 2018

We can determine which files are data migrations since they will define a class that inherits from ActiveRecord::Base. If we hook inherited in AR::Base, we can get the constant and the file in which it is defined (the data migration).

Adding this as an automated code review to address #4 (comment)

@bdunne bdunne force-pushed the data_migrations_need_tests branch 2 times, most recently from d485d17 to f930dbe Compare August 8, 2018 19:16

data_migration_files.each do |data_migration_file|
spec_file_basename = File.basename(data_migration_file).sub(".rb", "_spec.rb")
next if spec_file_basename.in?(KNOWN_MISSING_DATA_MIGRATION_SPEC_FILES)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think move this down and XOR the comparison, such that if someone adds a test that's in the missing list, they are forced to remove it.

@bdunne bdunne force-pushed the data_migrations_need_tests branch from f930dbe to 76a72cc Compare August 9, 2018 15:53
@bdunne bdunne closed this Aug 9, 2018
@bdunne bdunne reopened this Aug 9, 2018
Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but will merge after #249 , so @bdunne can you remove that from the KNOWN list?

end
end

ActiveRecord::Base.singleton_class.prepend(Spec::Support::ConstantWatcher)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you verify this works locally if you do bundle exec rspec spec/specific/spec/file.rb

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's loaded by spec_helper

@Fryguy
Copy link
Member

Fryguy commented Aug 9, 2018

#249 is merged, so please update the KNOWN list.

We can determine which files are data migrations since they will define a
class that inherits from ActiveRecord::Base.  If we hook inherited in
AR::Base, we can get the constant and the file in which it is defined
(the data migration).
@bdunne bdunne force-pushed the data_migrations_need_tests branch from 76a72cc to 622e6fa Compare August 9, 2018 17:12
@miq-bot
Copy link
Member

miq-bot commented Aug 9, 2018

Checked commit bdunne@622e6fa with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@Fryguy Fryguy merged commit 0fddbc7 into ManageIQ:master Aug 9, 2018
@Fryguy Fryguy added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 9, 2018
@bdunne bdunne deleted the data_migrations_need_tests branch August 9, 2018 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants