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

Add sensible defaults to active record objects #30

Closed
2 tasks
searls opened this issue Aug 22, 2016 · 2 comments
Closed
2 tasks

Add sensible defaults to active record objects #30

searls opened this issue Aug 22, 2016 · 2 comments

Comments

@searls
Copy link
Member

searls commented Aug 22, 2016

  • By default, (recursively?) compare attributes, excluding id, created_at, and updated_at.
  • initially implement with a devdep on AR, but replace it with a minimal fake once we're done to save on build time.
@searls searls added this to the Complex Type Support milestone Aug 22, 2016
searls added a commit that referenced this issue Sep 6, 2016
Run the test with:

    bundle exec rake suture

This was a little involved and my first-ever attempt using Suture with
ActiveRecord inputs & outputs. A few notes.

* The rake task is completely divorced from the Rails app & the built-in
Rails testing infrastructure/tasks. This seems like a good idea since
it'll probably often be the case that users will depend on a database
snapshot (or, at the very least, different database setup than the 
fixtures/factories used in the rest of their test suite)

* The test requires the Rails app as well as the item model

* The test setup has to establish an AR connection on its
own in UpdateQualityTest#setup, which is a little awkward

* The test's `:subject` lambda is intentionally identical to the 
one from ItemsController, so that we can validate that the recordings
can be replicated in a controlled automated environment just like they
were when we populated db/suture.sqlite3 via exploratory testing

* Because ActiveRecord compares items by id, the default comparator is
insufficient at least until #30 is resolved
@searls
Copy link
Member Author

searls commented Sep 6, 2016

Some thoughts: AR's default == behavior makes for a really bad out-of-the-box experience with Suture

I am truly happy to release a Suture 1.0.0 without any AR-specific affordances, but for the fact that by default users will get confusing false positives if they pass in or return AR objects to and from seams.

By default, AR will consider two already-persisted objects equivalent if their IDs match. This is definitely a problem with Suture'a default comparator, because drastically-mutated AR objects will be seen as "passing" in Suture.verify tests and as comparable in staging (e.g. call_both mode).

Since some woefully high percentage of legacy Ruby classes extend from AR::Base, this is a terrible out-of-the-box experience because it looks like it's passing immediately, the user must recognize it is not, and then figure out the Custom Comparator API to look at just the properties they care about.

This is unacceptably bad UX IMO.

@searls
Copy link
Member Author

searls commented Sep 6, 2016

My inclination at this point is to do a few things:

  • Add to the default comparator a check for defined?(ActiveRecord::Base) and if both objects are AR Base, enter a branch to compare them
  • By default, compare the two objects' attribute hashes
  • Expose a config property like :ignore_active_record_attributes which defaults to created_at and updated_at.
  • I have no idea how to handle associations for this purpose, though.

As an aside, I'm saying "ActiveRecord" here since attributes is defined in AR and not on ActiveModel. In practice I'm more interested in solving for the vast majority of cases rather than trying to build-in a super-smart comparator to serve everyone.

@searls searls closed this as completed in fc05602 Sep 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant