-
Notifications
You must be signed in to change notification settings - Fork 224
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
Test Against ActiveRecord #363
Conversation
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.
Looks good overall, however I think using a test matrix would make a bit more sense, and there are some simplifications available I think :)
@@ -7,3 +7,34 @@ gem "pg", "~> 1.0", platform: :ruby | |||
gem "activerecord-jdbcpostgresql-adapter", platform: :jruby | |||
gem "ffi-geos", platform: :jruby | |||
gem "byebug" if ENV["BYEBUG"] | |||
|
|||
def activerecord_version |
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.
This gets only one version, shouldn't we test against multiple rails versions ? For that, we could do as with ruby: take advantage of the test matrix in github ci.
In that case the version could simply be an env variable or a task argument
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.
Good point, I can add a check for an environment variable as well.
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'm not certain why having both is mandatory. Actually I think I am missing the point of this method :/
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.
Gotcha. The point of this method is that for local development if we want to test against ActiveRecord, we need to add Rails to the bundle and fetch it from its git repo in order to access all of the tests. If we just use the ActiveRecord from Rubygems the test directory is not included, same with Rails from Rubygems.
So this method reads in our gemspec, finds what ActiveRecord version we've specified as a dependency, fetches all of the Rails releases from Rubygems, finds the highest matching version (i.e. ActiveRecord ~> 7.0.0 resolves to ActiveRecord=7.0.2.3 currently), and then pulls the corresponding Rails version from git since all releases are tagged on git as well.
def load_postgis_specific_schema | ||
original_stdout = $stdout | ||
$stdout = StringIO.new | ||
|
||
load "schema/postgis_specific_schema.rb" | ||
|
||
ActiveRecord::FixtureSet.reset_cache | ||
ensure | ||
$stdout = original_stdout | ||
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.
Why this stdout stuff?
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 just copied from this.
It's not a big deal if we don't include the stdout logic, but it outputs a lot of migration logs if we don't.
test/rake_helper.rb
Outdated
def set_arconfig_env! | ||
arconfig_file = File.expand_path(File.join(File.dirname(__FILE__), "database.yml")) | ||
ENV["ARCONFIG"] = arconfig_file | ||
end | ||
|
||
config_load_paths! | ||
set_arconfig_env! | ||
|
||
def activerecord_test_files | ||
if ENV["AR_TEST_FILES"] |
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.
why not a single TEST_FILES
constant to have only one thing to remember ? :)
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.
It's possible that we will have conflicting files which could be confusing for a user. For example test/cases/type_test.rb
is defined in both postgis and ActiveRecord so it would run 2 test files if we only had one environment variable. Maybe it's not a big deal though.
Gemfile
Outdated
|
||
group :development do | ||
# Need to install for tests | ||
gem "rails", git: "https://github.com/rails/rails.git", tag: "v#{activerecord_version}" |
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.
gem "rails", git: "https://github.com/rails/rails.git", tag: "v#{activerecord_version}" | |
gem "rails", github: "rails/rails", tag: "v#{activerecord_version}" |
YAGNI but sexy ^^
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.
LGTM!
Some ActiveRecord specific tests are still failing, but we can handle those in separate issues/PRs as most of the remaining failures will require us to override/re-write the original test from Rails. |
Fixes #358
Add functionality to test the PostGIS adapter against the full ActiveRecord test suite for Postgres.
test/cases
and namespaced them withmodule PostGIS
to avoid potential conflicts.Gemfile
that pulls the matching Rails version to the ActiveRecord version specified in thegemspec
so we can access the tests.rake_helper
that help with getting the libs and tests from ActiveRecord.test/database.yml
to be in an ActiveRecord test compatible format.TestTask
s to theRakefile
that allow testing on just local PostGIS tests, ActiveRecord tests, or both. By default it is only local tests.bundle exec rake test:postgis
bundle exec rake test:activerecord
bundle exec rake test:all
TODOs: