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.

Sign up for GitHub

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

Jump to bottom

Fix tests of #405 "Bump to 7.2.0" #412

Merged
merged 4 commits into from
Oct 16, 2024
Merged

Fix tests of #405 "Bump to 7.2.0" #412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
35b0754
load current default value as is instead of making rgeo produce one. …
formigarafa Oct 14, 2024
e4613dd
fix rubocop layout warning on basic_test.rb
formigarafa Oct 14, 2024
b14c665
avoid using whole ar-testcase which causes trouble when running test:…
formigarafa Oct 14, 2024
f0ddf15
ensure connection is stablished
formigarafa Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions test/cases/basic_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ def test_version

def test_postgis_available
assert_equal "PostGIS", SpatialModel.lease_connection.adapter_name
assert_equal SpatialModel.lease_connection.select_value("SELECT postgis_lib_version()"), SpatialModel.lease_connection.postgis_lib_version
expected_postgis_lib_version_value = SpatialModel.lease_connection.select_value("SELECT postgis_lib_version()")
assert_equal expected_postgis_lib_version_value, SpatialModel.lease_connection.postgis_lib_version
valid_version = ["2.", "3."].any? { |major_ver| SpatialModel.lease_connection.postgis_lib_version.start_with? major_ver }
assert valid_version
end
Expand Down Expand Up @@ -146,7 +147,7 @@ def test_spatial_factory_attrs_parsing
has_z: false, has_m: false })

# wrong factory for default
old_default = spatial_factory_store.default
old_default = spatial_factory_store.instance_variable_get :@default
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is an ok behaviour in the lib (I mean if it is not clear when testing it, it may not be clear when using it), you've just work on it @formigarafa WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not like that either but there is no other way to infer if there is a default set in the factory store.

From rgeo-ativerecord: https://github.com/rgeo/rgeo-activerecord/blob/master/lib/rgeo/active_record/spatial_factory_store.rb#L20-L22

      def default(attrs = {})
        @default || default_for_attrs(attrs)
      end

Maybe If this method used ||= then it would work as the code seem to expect.

Copy link
Member

Choose a reason for hiding this comment

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

I'm unfortunately not sure it is that simple, for instance with ||= you'd get a weird behaviour doing:

f1 = store.default(some_attr)
f2 = stor.default(other_attr)

both would be the same, ignoring other_attr.

I opened an issue to see if there is a good way to refactor this rgeo/rgeo-activerecord#80

spatial_factory_store.default = RGeo::Geographic.spherical_factory(srid: 4326)

object = klass.new
Expand Down
5 changes: 4 additions & 1 deletion test/cases/ddl_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
# frozen_string_literal: true

require_relative "../test_helper"
require "active_record/testing/query_assertions"

module PostGIS
class DDLTest < ActiveRecord::TestCase
class DDLTest < ActiveSupport::TestCase
include ActiveRecord::Assertions::QueryAssertions

def test_spatial_column_options
[
:geography,
Expand Down
2 changes: 1 addition & 1 deletion test/cases/schema_statements_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ module PostGIS
class SchemaStatementsTest < ActiveSupport::TestCase
def test_initialize_type_map
SpatialModel.with_connection do |connection|
assert connection.connected?
connection.connect!
Copy link
Member

Choose a reason for hiding this comment

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

I guess the reasoning here is that connect! would raise if not able to connect? Do you have any idea why this has changed though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was added in #397 , and there is a comment about it: https://github.com/rgeo/activerecord-postgis-adapter/pull/397/files#r1646652915

I think this line was added to highlight why the next command was failing, maybe to try pinpoint the reason the connection sometimes is not there.
I found that this is also dependent of the order the tests run. If you run this test in isolation it would aways fail. If this test run after a test using the connection (eg: model.connection.create_table), then this test would pass.

initialized_types = connection.send(:type_map).keys

# PostGIS types must be initialized first, so
Expand Down