Fix tests of #405 "Bump to 7.2.0" #412
Conversation
…produced default is not memoized by rgeo.
formigarafa
changed the title
formigarafa
changed the title
test was failing when run in isolation
| @@ -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 | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
endMaybe If this method used ||= then it would work as the code seem to expect.
There was a problem hiding this comment.
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
| @@ -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! | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thinking about how this is wanted by the community, let's merge. I'll open an issue in which we'll discuss how to make these tests overall more reliable (thinking using fixtures for factories for instance). I'm still very interested in answering those questions though!
@keithdoggett all good for you too?
Fix tests setup and reset of #405:
Load current default spatial factory value as is instead of making rgeo produce one. Default factory produced by rgeo is not memoized by rgeo, but the set one is.
Resetting the default to a produced was fixing it to one fixed value.
Revert to use ActiveSupport::TestCase instead of ActiveRecord::TestCase on dd_test, it was causing the tests to attempt loading an misconfigured db connection.