Specs & Integration for ActiveRecord and ROM #37

nesaulov · 2017-10-27T21:41:19Z

Collections, scopes, associations etc

TODO:

ActiveRecord
ROM (v3)

houndci-bot · 2017-10-29T09:40:55Z

spec/build_schema_spec.rb

+
+
+
+


Extra blank line detected.

houndci-bot · 2017-10-29T09:40:56Z

spec/build_schema_spec.rb

+
+
+
+


Extra blank line detected.

coveralls · 2017-10-29T10:05:36Z

Coverage increased (+0.005%) to 99.583% when pulling 6ace23a on orm_specs into cf9cad5 on master.

…ize`

coveralls · 2017-10-29T15:05:23Z

Coverage increased (+0.005%) to 99.583% when pulling d2f5ec7 on orm_specs into cf9cad5 on master.

coveralls · 2017-10-29T15:05:23Z

Coverage increased (+0.005%) to 99.583% when pulling d2f5ec7 on orm_specs into cf9cad5 on master.

coveralls · 2017-10-29T15:21:23Z

Coverage increased (+0.005%) to 99.583% when pulling e9e5a31 on orm_specs into cf9cad5 on master.

# Conflicts: # spec/orms/ar.rb

houndci-bot · 2017-11-04T12:09:47Z

spec/orms/rom/instances_spec.rb

+# frozen_string_literal: true
+
+class UserModel
+  include Surrealist


include is used at the top level. Use inside class or module.

houndci-bot · 2017-11-04T12:09:47Z

spec/orms/rom/instances_spec.rb

+# frozen_string_literal: true
+
+class UserModel
+  include Surrealist


include is used at the top level. Use inside class or module.

nesaulov · 2017-11-06T16:47:37Z

@AlessandroMinali please review. I have decided to limit this PR to only AR and ROM because it might get too bloated. Some notes:

I've had to mess up Builder a bit because of AR::CollectionProxy serialization. I've done it so that Surrealist would work nicely with has_many associations:

class Human < ActiveRecord::Base
  include Surrealist
  
  has_many :children
  
  json_schema { { name: String, children: { age: Integer } } }
end

Human.first.surrealize
# => { name: 'John', children: [ { name: 'Jane', age: 15}, { name: 'Josh', age: 5 } ] }

I've had to mess up SchemaDefiner a bit because seems like instance variables are cleared with every call of ROM objects, so the only solution I see here is to define a class variable (which is not very good, but I don't know how to do it better - do you have any thoughts?)

AlessandroMinali · 2017-11-07T23:08:53Z

I'll look at this next week, currently on vacation. A quick scan looks good, but I'll want to try and pull out specific references to AR and ROM from the code.

# Conflicts: # .rubocop.yml

AlessandroMinali

Seems good but it's unfortunate(possibly unavoidable) that we have to make library specific patches.

AlessandroMinali · 2017-11-17T03:13:19Z

lib/surrealist.rb

+  # Class variable name that is set by SchemaDefiner
+  CLASS_VARIABLE = '@@__surrealist_schema'.freeze
+  # Struct to carry schema along
+  Schema = Struct.new(:key, :value).freeze


Should this bee at the top level, it is only ever used in Builder

Right, I'll place it in Builder.

AlessandroMinali · 2017-11-17T06:17:33Z

lib/surrealist/builder.rb

+    # @param [Symbol] method
+    #
+    # @return [Boolean]
+    def ar_collection?(instance, method)


I tried to remove this...but with no success, this will probably become pretty nasty with more integrations. Seems to be an issue caused only when joining queries(?)

Yeah, that's unfortunate, but I didn't find a solution to escape this check. It's caused by ActiveRecord::Associations::AssociationProxy

AlessandroMinali · 2017-11-17T06:19:42Z

lib/surrealist/schema_definer.rb

@@ -15,7 +15,11 @@ class SchemaDefiner
    def self.call(klass, hash)
      raise Surrealist::InvalidSchemaError, 'Schema should be defined as a hash' unless hash.is_a?(Hash)

-      klass.instance_variable_set('@__surrealist_schema', hash)
+      if klass.name =~ /ROM::Struct/
+        klass.class_variable_set('@@__surrealist_schema', hash)


This will probably cause undesired behavior with ROM(?)

Probably need similar spec for ROM similar to this one.

surrealist/spec/build_schema_spec.rb

Lines 192 to 198 in 0a8a7ad

context 'inheritance of class that has delegated but we don\'t delegate' do

it 'raises RuntimeError' do

expect { FriendOfGuest.new.build_schema }

.to raise_error(Surrealist::UnknownSchemaError,

"Can't serialize FriendOfGuest - no schema was provided.")

end

end

I guess this brings up the problem of what is the minimum expected behaviour we want to test since we are not fully replicating all tests for each ORM.

I've added such spec. Talking about minimal behaviour to test, I think that ROM is pretty much covered right now, though it may seem that there are not so many examples there. AR tests are pretty massive because of scopes

AlessandroMinali

I added a few more specs to cover some deeper examples of what I was worried about.

#49

add spec to cover grandchild delegation subtleties

nesaulov · 2017-11-22T18:43:43Z

@AlessandroMinali thanks!

nesaulov added 6 commits October 18, 2017 23:50

WIP

162bcbb

[ActiveRecord] add tests for scopes

9673e7d

[ActiveRecord] Finish specs for AR

4d56ce8

Clean up a bit

3d53bc2

Add gemfile for ruby 2.2

328915e

Add tweaks for AR 4.2 & ruby 2.2.0

b5a2c08

Repository owner deleted a comment from coveralls Oct 28, 2017

nesaulov changed the title ~~[WIP] Specs for all ORMs~~ [WIP] Specs & Integration for all ORMs Oct 28, 2017

nesaulov added 2 commits October 28, 2017 16:29

Fix 👮‍♂️ configuration (maybe)

ad20095

Switch from rake rubocop task to hound ci

6ace23a

houndci-bot reviewed Oct 29, 2017

View reviewed changes

spec/build_schema_spec.rb Outdated

Copy link

houndci-bot Oct 29, 2017

Choose a reason for hiding this comment

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

Extra blank line detected.

houndci-bot reviewed Oct 29, 2017

View reviewed changes

spec/build_schema_spec.rb Outdated

Copy link

houndci-bot Oct 29, 2017

Choose a reason for hiding this comment

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

Extra blank line detected.

Repository owner deleted a comment from houndci-bot Oct 29, 2017

[ActiveRecord] Add specs for scopes & associations for `Model.surreal…

d2f5ec7

…ize`

[ActiveRecord] Fix NoMethodError message

e9e5a31

nesaulov added 4 commits November 3, 2017 10:09

Merge branch 'master' into orm_specs

f639b18

Patch ValueAssigner in a bit nasty way

fa52e9d

Revert changes in ValueAssigner and mess up Builder again

7437e81

Merge branch 'master' into orm_specs

523d3ef

# Conflicts: # spec/orms/ar.rb

Repository owner deleted a comment from houndci-bot Nov 4, 2017

[ROM] Improve instances spec

bdde281

houndci-bot reviewed Nov 4, 2017

View reviewed changes

nesaulov added 4 commits November 4, 2017 15:11

Add workaround for Rubocop bug

ef46072

[ROM] Add collection specs

502ce4c

Refactor Builder

5e15067

Minor refactoring

04a12d7

nesaulov changed the title ~~[WIP] Specs & Integration for all ORMs~~ [WIP] Specs & Integration for ActiveRecord and ROM Nov 6, 2017

nesaulov requested a review from AlessandroMinali November 7, 2017 18:34

nesaulov mentioned this pull request Nov 16, 2017

[WIP] Sequel integration #47

Merged

Merge branch 'master' into orm_specs

131b3c7

# Conflicts: # .rubocop.yml

Repository owner deleted a comment from houndci-bot Nov 16, 2017

AlessandroMinali reviewed Nov 17, 2017

View reviewed changes

nesaulov added 2 commits November 18, 2017 21:37

Move Schema to Builder

9733bb6

Add specs for ROM class without schema

de2a0a7

nesaulov changed the title ~~[WIP] Specs & Integration for ActiveRecord and ROM~~ Specs & Integration for ActiveRecord and ROM Nov 18, 2017

nesaulov requested a review from AlessandroMinali November 21, 2017 21:26

add spec to cover grandchild delegation subtleties

9c3bc02

AlessandroMinali approved these changes Nov 22, 2017

View reviewed changes

Merge pull request #49 from AlessandroMinali/orm_specs

1e7a976

add spec to cover grandchild delegation subtleties

nesaulov merged commit 4ea3723 into master Nov 22, 2017

nesaulov deleted the orm_specs branch November 22, 2017 18:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specs & Integration for ActiveRecord and ROM #37

Specs & Integration for ActiveRecord and ROM #37

nesaulov commented Oct 27, 2017 •

edited

Loading

houndci-bot Oct 29, 2017

houndci-bot Oct 29, 2017

coveralls commented Oct 29, 2017 •

edited

Loading

coveralls commented Oct 29, 2017 •

edited

Loading

coveralls commented Oct 29, 2017

coveralls commented Oct 29, 2017 •

edited

Loading

houndci-bot Nov 4, 2017

houndci-bot Nov 4, 2017

nesaulov commented Nov 6, 2017

AlessandroMinali commented Nov 7, 2017

AlessandroMinali left a comment

AlessandroMinali Nov 17, 2017

nesaulov Nov 18, 2017

AlessandroMinali Nov 17, 2017 •

edited

Loading

nesaulov Nov 18, 2017

AlessandroMinali Nov 17, 2017 •

edited

Loading

nesaulov Nov 18, 2017

AlessandroMinali left a comment

nesaulov commented Nov 22, 2017

	context 'inheritance of class that has delegated but we don\'t delegate' do
	it 'raises RuntimeError' do
	expect { FriendOfGuest.new.build_schema }
	.to raise_error(Surrealist::UnknownSchemaError,
	"Can't serialize FriendOfGuest - no schema was provided.")
	end
	end

Specs & Integration for ActiveRecord and ROM #37

Specs & Integration for ActiveRecord and ROM #37

Conversation

nesaulov commented Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 29, 2017 • edited Loading

coveralls commented Oct 29, 2017 • edited Loading

coveralls commented Oct 29, 2017

coveralls commented Oct 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nesaulov commented Nov 6, 2017

AlessandroMinali commented Nov 7, 2017

AlessandroMinali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlessandroMinali Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlessandroMinali Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlessandroMinali left a comment

Choose a reason for hiding this comment

nesaulov commented Nov 22, 2017

nesaulov commented Oct 27, 2017 •

edited

Loading

coveralls commented Oct 29, 2017 •

edited

Loading

coveralls commented Oct 29, 2017 •

edited

Loading

coveralls commented Oct 29, 2017 •

edited

Loading

AlessandroMinali Nov 17, 2017 •

edited

Loading

AlessandroMinali Nov 17, 2017 •

edited

Loading