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

Bump to 7.2.0 #405

Merged
merged 33 commits into from
Nov 4, 2024
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
a62542d
feat: bump activerecord
BuonOmo Sep 4, 2024
c21dbce
feat: enable all tests
BuonOmo Sep 4, 2024
28cab3a
fix: use lease_connection in tests
BuonOmo Sep 4, 2024
b4ed5bf
add tracer to figure it out
BuonOmo Sep 5, 2024
bc31a28
stackprof less verbose
BuonOmo Sep 5, 2024
5fd9f6c
enable_ext analysis
BuonOmo Sep 5, 2024
3be96f9
enable_ext stackprof
BuonOmo Sep 5, 2024
eee50f6
show conninfo
BuonOmo Sep 5, 2024
ec1c5ef
debug conninfo
BuonOmo Sep 5, 2024
342fcb2
maybe reset is the culprit
BuonOmo Sep 5, 2024
4951a6b
closer
BuonOmo Sep 5, 2024
b774ae2
fixed maybe
BuonOmo Sep 5, 2024
d1a4e1e
cleanup
BuonOmo Sep 5, 2024
2eef5aa
add exclusions
BuonOmo Sep 5, 2024
bbc38b4
fix structure dump test
BuonOmo Sep 5, 2024
9fb267c
add exclusions
BuonOmo Sep 5, 2024
69012e3
cleanup
BuonOmo Sep 5, 2024
ada1f96
longer timeout
BuonOmo Sep 9, 2024
0064eea
more excludes
BuonOmo Sep 9, 2024
9dab4f5
more excludes
BuonOmo Sep 9, 2024
eea4b76
fix flaky tests
BuonOmo Sep 9, 2024
763cef8
bump rgeo-activerecord
BuonOmo Sep 15, 2024
acbe7c8
debug info
BuonOmo Oct 7, 2024
485ccb5
fixing maybe
BuonOmo Oct 7, 2024
7d58504
more debug info
BuonOmo Oct 7, 2024
7b13ba6
intent tracer usage
BuonOmo Oct 7, 2024
42e8e55
no tracer
BuonOmo Oct 7, 2024
956e5f8
Fix tests of #405 "Bump to 7.2.0" (#412)
formigarafa Oct 16, 2024
4f6ebe6
fix test teardown (#413)
formigarafa Oct 17, 2024
04dd527
typos
BuonOmo Oct 17, 2024
ac74f4e
chore: add fundings
BuonOmo Oct 7, 2024
435027d
cleanup
BuonOmo Oct 17, 2024
ecf8ef3
Bump version, update history and README
keithdoggett Nov 4, 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
1 change: 1 addition & 0 deletions .github/FUNDING.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
open_collective: rgeo
38 changes: 32 additions & 6 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,25 @@ on:
branches:
- master
pull_request:
types: [opened, reopened, synchronize]
# Allows you to run this workflow manually from the Actions tab
workflow_dispatch:

# This allows a subsequently queued workflow run to interrupt previous runs.
concurrency:
group: "${{ github.workflow }} @ ${{ github.ref }}"
cancel-in-progress: true

jobs:
test-ubuntu-ruby:
# Since the name of the matrix job depends on the version, we define another job with a more stable name.
test_results:
if: ${{ always() }}
runs-on: ubuntu-latest
name: Test Results
needs: [test]
steps:
- run: '[[ "${{ needs.test.result }}" == "success" ]]'
test:
runs-on: ubuntu-latest
services:
postgis:
Expand All @@ -27,7 +43,9 @@ jobs:
strategy:
fail-fast: false
matrix:
ruby: [ruby-head, '3.3', '3.2', '3.1']
# https://ruby-lang.org/en/downloads/branches
ruby: ["3.3", "3.2", "3.1"]
# https://www.postgresql.org/support/versioning/
pg: [12-master, 13-master, 14-master, 15-master, 16-master]
steps:
- name: Set Up Actions
Expand All @@ -39,12 +57,20 @@ jobs:
with:
ruby-version: ${{ matrix.ruby }}
bundler-cache: true
- name: Create Database
run: psql -d postgresql://postgres:postgres@localhost:5432/postgres -c "create database postgis_adapter_test"
- name: Create PostGIS Extension
run: psql -d postgresql://postgres:postgres@localhost:5432/postgis_adapter_test -c "create extension postgis"
- name: Setup Database
run: |
psql -d postgresql://postgres:postgres@localhost:5432/postgres \
-c "create database postgis_adapter_test" \
-c "create database activerecord_unittest" \
-c "create database activerecord_unittest2"
for db in postgis_adapter_test activerecord_unittest activerecord_unittest2; do
psql -d postgresql://postgres:postgres@localhost:5432/$db -c "create extension postgis"
done
- name: Run Tests
run: bundle exec rake test
env:
PGHOST: localhost
PGUSER: postgres
PGPASSWORD: postgres
TESTOPTS: --profile=3
TEST_TIMEOUT: 30
2 changes: 1 addition & 1 deletion Rakefile
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ require "rake/testtask"
require_relative "test/rake_helper"

task default: [:test]
task test: "test:postgis"
task test: "test:all"
Copy link
Contributor

Choose a reason for hiding this comment

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

i think the whole suite is flaky.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll look at it. I have good experience with this suite now that I maintain the CRDB adapter. It usually is flaky due to some tests modifying the db. Which they should not do if they are correctly changed for our use case. Do you have any further information?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just this : #378

Copy link
Member Author

Choose a reason for hiding this comment

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

Thx, I plan on excluding using minitest-exclude and opening an issue for excluded tests

Copy link
Member

Choose a reason for hiding this comment

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

@BuonOmo yeah I hadn't figured out how to replicate the exclusions like in the CRDB adapter, but we definitely need that here since some are failing (which is expected). But to @seuros 's point that's why I just left the postgis tests for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now we only have Postgis basic tests flaky. If one of you has time to look into it it would be much appreciated. Otherwise maybe we can skip them for now and make this or top priority issue, just so we can release this ?

Copy link
Member

Choose a reason for hiding this comment

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

@BuonOmo so just to make sure I'm clear, there's a few postgis tests that were working and are now flakey? I can try to work through some of them. Is it only in the CI?

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed some of the tests failures are order related.
notes here: #405 (comment)


Rake::TestTask.new(:test_postgis) do |t|
t.libs << postgis_test_load_paths
Expand Down
10 changes: 6 additions & 4 deletions activerecord-postgis-adapter.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -10,25 +10,27 @@ Gem::Specification.new do |spec|

spec.version = ActiveRecord::ConnectionAdapters::PostGIS::VERSION
spec.authors = ["Daniel Azuma", "Tee Parham"]
seuros marked this conversation as resolved.
Show resolved Hide resolved
spec.email = ["dazuma@gmail.com", "parhameter@gmail.com", "kfdoggett@gmail.com"]
spec.email = ["kfdoggett@gmail.com", "buonomo.ulysse@gmail.com", "terminale@gmail.com"]
spec.homepage = "http://github.com/rgeo/activerecord-postgis-adapter"
spec.license = "BSD-3-Clause"

spec.files = Dir["lib/**/*", "LICENSE.txt"]
spec.platform = Gem::Platform::RUBY

# ruby-lang.org/en/downloads/branches
spec.required_ruby_version = ">= 3.1.0"

spec.add_dependency "activerecord", "~> 7.2.0.beta2"
spec.add_dependency "rgeo-activerecord", "~> 7.0.0"
spec.add_dependency "activerecord", "~> 7.2.0"
spec.add_dependency "rgeo-activerecord", "~> 8.0.0"

spec.add_development_dependency "rake", "~> 13.0"
spec.add_development_dependency "minitest", "~> 5.4"
spec.add_development_dependency "mocha", "~> 2.4"
spec.add_development_dependency "minitest-excludes", "~> 2.0"
spec.add_development_dependency "benchmark-ips", "~> 2.12"
spec.add_development_dependency "rubocop", "~> 1.50"

spec.metadata = {
"funding_uri" => "https://opencollective.com/rgeo",
"rubygems_mfa_required" => "true"
}
end
33 changes: 33 additions & 0 deletions bin/console
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
#!/usr/bin/env ruby

$:.unshift(File.expand_path("../lib", __dir__))

# require "bundler/setup"
# Bundler.require :development
#
require "activerecord-postgis-adapter"

db_name = "activerecord_postgis_adapter_console"
system("psql -c 'drop database if exists #{db_name}' postgres",
exception: true)
system("psql -c 'create database #{db_name}' postgres",
exception: true)
system(psql = "psql -c 'create extension postgis' #{db_name} ", exception: true)

ActiveRecord::Base.establish_connection(
adapter: "postgis",
database: db_name
)

class Country < ActiveRecord::Base
end

ActiveRecord::Schema.define do
create_table(:countries) do |t|
t.st_polygon(:area, srid: 4326)
t.string(:name)
end
end

require "irb"
IRB.start(__FILE__)
27 changes: 0 additions & 27 deletions lib/active_record/connection_adapters/postgis/create_connection.rb

This file was deleted.

1 change: 0 additions & 1 deletion lib/active_record/connection_adapters/postgis_adapter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
require_relative "postgis/oid/spatial"
require_relative "postgis/oid/date_time"
require_relative "postgis/type" # has to be after oid/*
require_relative "postgis/create_connection"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only actual needed change according to the current test suite, but I'm trying with the whole rails test suite to be sure we do not miss anything

# :startdoc:

module ActiveRecord
Expand Down
2 changes: 1 addition & 1 deletion lib/activerecord-postgis-adapter.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# frozen_string_literal: true

require 'active_record'
require "active_record"
require "active_record/connection_adapters"
require "rgeo/active_record"
ActiveRecord::ConnectionAdapters.register("postgis", "ActiveRecord::ConnectionAdapters::PostGISAdapter", "active_record/connection_adapters/postgis_adapter")
7 changes: 3 additions & 4 deletions test/cases/attributes_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ class InvalidAttribute < ActiveRecord::Base

class AttributesTest < ActiveSupport::TestCase
def setup
reset_spatial_store
create_foo
create_spatial_foo
create_invalid_attributes
Expand Down Expand Up @@ -99,20 +98,20 @@ def test_joined_spatial_attribute
private

def create_foo
Foo.connection.create_table :foos, force: true do |t|
Foo.lease_connection.create_table :foos, force: true do |t|
Copy link
Contributor

Choose a reason for hiding this comment

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

these test case work in isolation

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean if you run any test that break alone, it works.

Something wrong with CI that we should fix.
Checkout https://github.com/nektos/act if you want to have your own github runner locally.

Copy link
Member Author

@BuonOmo BuonOmo Sep 5, 2024

Choose a reason for hiding this comment

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

Thanks for act, I am still unsure about your first point though..

end
end

def create_spatial_foo
SpatialFoo.connection.create_table :spatial_foos, force: true do |t|
SpatialFoo.lease_connection.create_table :spatial_foos, force: true do |t|
t.references :foo
t.st_point :geo_point, geographic: true, srid: 4326
t.st_point :cart_point, srid: 3509
end
end

def create_invalid_attributes
InvalidAttribute.connection.create_table :invalid_attributes, force: true do |t|
InvalidAttribute.lease_connection.create_table :invalid_attributes, force: true do |t|
end
end
end
Expand Down
25 changes: 13 additions & 12 deletions test/cases/basic_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

module PostGIS
class BasicTest < ActiveSupport::TestCase
def before
def teardown
reset_spatial_store
end

Expand All @@ -13,22 +13,23 @@ def test_version
end

def test_postgis_available
assert_equal "PostGIS", SpatialModel.connection.adapter_name
assert_equal postgis_version, SpatialModel.connection.postgis_lib_version
valid_version = ["2.", "3."].any? { |major_ver| SpatialModel.connection.postgis_lib_version.start_with? major_ver }
assert_equal "PostGIS", SpatialModel.lease_connection.adapter_name
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

def test_arel_visitor
visitor = Arel::Visitors::PostGIS.new(SpatialModel.connection)
visitor = Arel::Visitors::PostGIS.new(SpatialModel.lease_connection)
node = RGeo::ActiveRecord::SpatialConstantNode.new("POINT (1.0 2.0)")
collector = Arel::Collectors::PlainString.new
visitor.accept(node, collector)
assert_equal "ST_GeomFromText('POINT (1.0 2.0)')", collector.value
end

def test_arel_visitor_will_not_visit_string
visitor = Arel::Visitors::PostGIS.new(SpatialModel.connection)
visitor = Arel::Visitors::PostGIS.new(SpatialModel.lease_connection)
node = "POINT (1 2)"
collector = Arel::Collectors::PlainString.new

Expand Down Expand Up @@ -111,7 +112,7 @@ def test_default_value

def test_custom_factory
klass = SpatialModel
klass.connection.create_table(:spatial_models, force: true) do |t|
klass.lease_connection.create_table(:spatial_models, force: true) do |t|
t.st_polygon(:area, srid: 4326)
end
klass.reset_column_information
Expand All @@ -127,7 +128,7 @@ def test_custom_factory

def test_spatial_factory_attrs_parsing
klass = SpatialModel
klass.connection.create_table(:spatial_models, force: true) do |t|
klass.lease_connection.create_table(:spatial_models, force: true) do |t|
t.multi_polygon(:areas, srid: 4326)
end
klass.reset_column_information
Expand All @@ -152,14 +153,14 @@ def test_readme_example
spatial_factory_store.register(geo_factory, geo_type: "point", sql_type: "geography")

klass = SpatialModel
klass.connection.create_table(:spatial_models, force: true) do |t|
klass.lease_connection.create_table(:spatial_models, force: true) do |t|
t.column(:shape, :geometry)
t.line_string(:path, srid: 3785)
t.st_point(:latlon, geographic: true)
end
klass.reset_column_information
assert_includes klass.columns.map(&:name), "shape"
klass.connection.change_table(:spatial_models) do |t|
klass.lease_connection.change_table(:spatial_models) do |t|
t.index(:latlon, using: :gist)
end

Expand Down Expand Up @@ -192,7 +193,7 @@ def test_custom_column
end

def test_multi_polygon_column
SpatialModel.connection.create_table(:spatial_models, force: true) do |t|
SpatialModel.lease_connection.create_table(:spatial_models, force: true) do |t|
t.column "m_poly", :multi_polygon
end
SpatialModel.reset_column_information
Expand All @@ -212,7 +213,7 @@ def test_multi_polygon_column
private

def create_model
SpatialModel.connection.create_table(:spatial_models, force: true) do |t|
SpatialModel.lease_connection.create_table(:spatial_models, force: true) do |t|
t.column "latlon", :st_point, srid: 3785
t.column "latlon_geo", :st_point, srid: 4326, geographic: true
t.column "default_latlon", :st_point, srid: 0, default: "POINT(0.0 0.0)"
Expand Down
Loading