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

Split spec/support/active_record_classes.rb #342

Merged
merged 6 commits into from
Feb 29, 2024

Conversation

ellnix
Copy link
Collaborator

@ellnix ellnix commented Feb 28, 2024

A first step toward #332

@ellnix ellnix added skip-changelog The PR will not appear in the release changelogs maintenance Anything related to maintenance (CI, tests, refactoring...) labels Feb 28, 2024
Copy link

codecov bot commented Feb 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.22%. Comparing base (b5983bd) to head (d00f686).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #342   +/-   ##
=======================================
  Coverage   90.22%   90.22%           
=======================================
  Files          13       13           
  Lines         757      757           
=======================================
  Hits          683      683           
  Misses         74       74           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ellnix ellnix force-pushed the split-integration-spec branch from fbe9e48 to a4c1d13 Compare February 28, 2024 19:02
Copy link
Collaborator Author

@ellnix ellnix left a comment

Choose a reason for hiding this comment

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

I was intending to also move the rest of integration_spec in this PR but that looked like it was going to involve a lot more edits.

There aren't too many changes in this one despite what the diff says xD

.rubocop.yml Show resolved Hide resolved
spec/integration_spec.rb Show resolved Hide resolved
spec/spec_helper.rb Show resolved Hide resolved
spec/support/active_record_classes.rb Show resolved Hide resolved
spec/support/active_record_classes.rb Show resolved Hide resolved
spec/support/active_record_classes.rb Show resolved Hide resolved
ActiveJob::Base.queue_adapter = :test
end

def ar_schema
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This method is responsible for decentralizing ActiveRecord::Schema.define.

Since the old define method mainly instance_exec'ed the block provided to it, this should be fine (define also updated some metadata tables but they should not be necessary for us).

@@ -0,0 +1,33 @@
require 'support/active_record_schema'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These requires I thought would make it obvious where ar_schema comes from, so it's easier to follow the logic in an integration spec from
require 'support/models/<model>
to
require 'support/active_record_schema
in a few clicks and get a grasp of what is happening.

spec/support/sequel_db.rb Show resolved Hide resolved
@ellnix ellnix changed the title Split integration spec Split spec/support/active_record_classes.rb Feb 28, 2024
@ellnix ellnix marked this pull request as ready for review February 28, 2024 19:23
@ellnix ellnix requested a review from brunoocasali February 28, 2024 19:23
Debated removing this model entirely.
@ellnix ellnix mentioned this pull request Feb 29, 2024
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

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

LGTM! Very good work @ellnix

That's the perfect balance between the size of the PR and the changes itself. Very easy to grasp what's going on.

Keep going 🤘

@ellnix
Copy link
Collaborator Author

ellnix commented Feb 29, 2024

bors merge

@meili-bors meili-bors bot merged commit 7459bde into meilisearch:main Feb 29, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Anything related to maintenance (CI, tests, refactoring...) skip-changelog The PR will not appear in the release changelogs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants