Updated Rails, Fixed Tests, Updated Readme #34

natesire · 2018-04-12T21:43:26Z

Changes Made

Updated Readme to explain the sql output file is stored in the db directory. I was looking in the project root directory.
Updated the gem to use the latest active record version.
Fixed a faulty test that was searching an array for added sql statements.

brandonjoyce

Thanks for contributing! Just a few suggestions. Let me know what you think.

brandonjoyce · 2018-04-13T13:49:20Z

spec/sql_footprint_spec.rb

@@ -28,14 +29,14 @@
      )
    end

+    it 'formats selects by 1 count' do


This test isn't checking the formatting so this might be a better name:
it 'appends 1 statement'

brandonjoyce · 2018-04-13T13:51:33Z

spec/sql_footprint_spec.rb

-        'ORDER BY "widgets"."id" DESC LIMIT 1'
-      )
+      ap statements.to_a
+      expect(statements.to_a.to_s).to include '\"widgets\".\"name\" = ?'


I think it might be better to leave this test as it was before. Since we're testing the formatting of the output, I kinda like the thoroughness of looking for the whole statement.

brandonjoyce · 2018-04-13T13:52:21Z

sql_footprint.gemspec

@@ -34,4 +34,5 @@ Gem::Specification.new do |spec|
  spec.add_development_dependency 'sqlite3'
  spec.add_development_dependency 'rubocop', '~> 0.37.0'
  spec.add_development_dependency 'rubocop-rspec'
+  spec.add_development_dependency 'awesome_print'


Can we kill this before merging? I'm not sure it adds enough value to stay.

brandonjoyce · 2018-04-13T13:57:34Z

Oh, also looks like failing on CI if you don't mind checking what's up with that.

natesire · 2018-04-13T15:21:10Z

Brandon, Thanks for quick reply. Always happy to contribute. 1. Yes, much better test name using the word append instead of format :) 2. Yes, I will remove the awesome print gem. No need to add an extra dependency. 3. I will double check the CI/Travis. The only change I might differ on opinion is... The larger sql format test versus using smaller tests. It's more difficult to figure out what is failing when the test is larger. It's better to have smaller tests so you know exactly what is failing by just reading a few lines. We could add the longer more thorough test back at the end of some smaller tests. Think of the larger test as how integration tests work where they test multiple things after testing little unit blocks. And everyone knows units tests are almost always easier to debug than integration tests. Let me know your thoughts and if you disagree. Also, smaller tests are less brittle.

…

-Nathan

On Friday, April 13, 2018, Brandon Joyce ***@***.***> wrote: ***@***.**** requested changes on this pull request. Thanks for contributing! Just a few suggestions. Let me know what you think. ------------------------------ In spec/sql_footprint_spec.rb <#34 (comment)> : > @@ -28,14 +29,14 @@ ) end + it 'formats selects by 1 count' do This test isn't checking the formatting so this might be a better name: it 'appends 1 statement' ------------------------------ In spec/sql_footprint_spec.rb <#34 (comment)> : > it 'formats selects' do Widget.where(name: SecureRandom.uuid, quantity: 1).last - expect(statements.to_a).to include( - 'SELECT "widgets".* FROM "widgets" ' \ - 'WHERE "widgets"."name" = ? AND ' \ - '"widgets"."quantity" = ? ' \ - 'ORDER BY "widgets"."id" DESC LIMIT 1' - ) + ap statements.to_a + expect(statements.to_a.to_s).to include '\"widgets\".\"name\" = ?' I think it might be better to leave this test as it was before. Since we're testing the formatting of the output, I kinda like the thoroughness of looking for the whole statement. ------------------------------ In sql_footprint.gemspec <#34 (comment)> : > @@ -34,4 +34,5 @@ Gem::Specification.new do |spec| spec.add_development_dependency 'sqlite3' spec.add_development_dependency 'rubocop', '~> 0.37.0' spec.add_development_dependency 'rubocop-rspec' + spec.add_development_dependency 'awesome_print' Can we kill this before merging? I'm not sure it adds enough value to stay. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#34 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEXKPAlN3mzupq5Wg7faj7osgTS_VK9Lks5toK2zgaJpZM4TSihP> .

brandonjoyce · 2018-04-13T19:47:16Z

I agree with your statements on the differences between high and low level tests, but I'm thinking of specs/sql_footprint_spec.rb as the high level integration tests while most of the smaller scope tests are in the spec/sql_footprint/ folder.

Here's where the high level version provides value: It looks like the new version of ActiveRecord changed the ordering of the sql statement syntax which caused the long form test to fail for you. I think that's important feedback to get from the test suite even if it's a little harder to read. With that information we can inform users that an upgrade will cause some churn in their footprint files ahead of time.

I'm totally ok with adding some more granular tests as well though if you're looking for specific things.

Disclaimer: It's been quite a while since I've updated anything in the project so I'm open to hearing that I'm wrong. 😄

natesire · 2018-04-13T19:50:06Z

Brandon, Great. I agree, I like more thorough tests too. I will fix up the changes. I always learn something new too when reading other's code. I never thought to use a +1 and -1 when checking for the change count test.

…

-Nathan

On Friday, April 13, 2018, Brandon Joyce ***@***.***> wrote: I agree with your statements on the differences between high and low level tests, but I'm thinking of specs/sql_footprint_spec.rb as the high level integration tests while most of the smaller scope tests are in the spec/sql_footprint/ folder. *Here's where the high level version provides value*: It looks like the new version of ActiveRecord changed the ordering of the sql statement syntax which caused the long form test to fail for you. I think that's important feedback to get from the test suite even if it's a little harder to read. With that information we can inform users that an upgrade will cause some churn in their footprint files ahead of time. I'm totally ok with adding some more granular tests as well though if you're looking for specific things. *Disclaimer:* It's been quite a while since I've updated anything in the project so I'm open to hearing that I'm wrong. 😄 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#34 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEXKPM5GWkVE3krlUPQnPpzIPEVbqemgks5toQDHgaJpZM4TSihP> .

mikegee · 2018-04-13T20:19:20Z

README.md

@@ -65,7 +65,8 @@ Or if you're using FactoryGirl you could do something like this:
 RSpec.configure do |config|
  module FactoryBoy
    def create(*args)
-      SqlFootprint.exclude { FactoryGirl.create(*args) }
+      ``` FactoryGirl changed their gem name to FactoryBot ```


hrm ... FactoryBoy is no longer a very good name for this module. It was meant as a companion to FactoryGirl, now it looks like a typo.

natesire · 2018-04-13T20:25:00Z

Nice catch. How about FactoryBotKid?

…

On Friday, April 13, 2018, Michael Gee ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In README.md <#34 (comment)> : > @@ -65,7 +65,8 @@ Or if you're using FactoryGirl you could do something like this: RSpec.configure do |config| module FactoryBoy def create(*args) - SqlFootprint.exclude { FactoryGirl.create(*args) } + ``` FactoryGirl changed their gem name to FactoryBot ``` hrm ... FactoryBoy is no longer a very good name for this module. It was meant as a companion to FactoryGirl, now it looks like a typo. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#34 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEXKPNMPWvhMT_KV10ckU4a_9YPphiJeks5toQhLgaJpZM4TSihP> .

natesire · 2018-04-14T02:17:33Z

Brandon, I made the changes. I also narrowed most of the dependency gems down to minimum version which is good practice.

…

-Nathan On Fri, Apr 13, 2018 at 4:24 PM, nathan sire <[email protected]> wrote:

Nice catch. How about FactoryBotKid? On Friday, April 13, 2018, Michael Gee ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In README.md > <#34 (comment)> > : > > > @@ -65,7 +65,8 @@ Or if you're using FactoryGirl you could do something like this: > RSpec.configure do |config| > module FactoryBoy > def create(*args) > - SqlFootprint.exclude { FactoryGirl.create(*args) } > + ``` FactoryGirl changed their gem name to FactoryBot ``` > > hrm ... FactoryBoy is no longer a very good name for this module. It was > meant as a companion to FactoryGirl, now it looks like a typo. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#34 (review)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AEXKPNMPWvhMT_KV10ckU4a_9YPphiJeks5toQhLgaJpZM4TSihP> > . >

AlexMooney · 2018-05-25T22:12:55Z

.gitignore

@@ -9,3 +9,6 @@
 /tmp/
 footprint.sql
 footprint.:memory:.sql
+.idea/


It would be better to add this to your .gitignore_global file rather than checking in intelliJ specific stuff here. Presumably the same applies to the rbenv stuff.

natesire · 2018-06-06T16:52:48Z

I updated the pull request.

…

On Friday, May 25, 2018, Alex Mooney ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In .gitignore <#34 (comment)> : > @@ -9,3 +9,6 @@ /tmp/ footprint.sql footprint.:memory:.sql +.idea/ It would be better to add this to your .gitignore_global file rather than checking in intelliJ specific stuff here. Presumably the same applies to the rbenv stuff. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#34 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEXKPE3c_-DpUswhX5ZFoOgR-vGDEOkcks5t2IHpgaJpZM4TSihP> .

mikegee · 2018-06-06T18:33:50Z

.gitignore

@@ -9,3 +9,5 @@
 /tmp/
 footprint.sql
 footprint.:memory:.sql
+.rbenv-gemsets
+.gems


I think these two things might be specific to your tool choices, too. Or, do you think other devs will be installing gems in .gems too?

mikegee · 2018-06-06T18:38:47Z

spec/sql_footprint_spec.rb

+      matches.push 'ORDER BY "widgets"."id" DESC'
+      matches.push 'LIMIT ?'
+
+      puts statements.to_a.to_s


Should we remove this puts before merging?

natesire · 2018-06-06T18:48:33Z

Ya, I'll remove that. Left over debug code.

…

On Wednesday, June 6, 2018, Michael Gee ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In spec/sql_footprint_spec.rb <#34 (comment)> : > Widget.where(name: SecureRandom.uuid, quantity: 1).last - expect(statements.to_a).to include( - 'SELECT "widgets".* FROM "widgets" ' \ - 'WHERE "widgets"."name" = ? AND ' \ - '"widgets"."quantity" = ? ' \ - 'ORDER BY "widgets"."id" DESC LIMIT 1' - ) + + matches.push 'SELECT "widgets".* FROM "widgets"' + matches.push 'WHERE "widgets"."name" = ?' + matches.push 'AND' + matches.push 'widgets"."quantity" = ?' + matches.push 'ORDER BY "widgets"."id" DESC' + matches.push 'LIMIT ?' + + puts statements.to_a.to_s Should we remove this puts before merging? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#34 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEXKPNY4xOWygx-7jN5iBp-YwlbftQkTks5t6CG5gaJpZM4TSihP> .

mikegee · 2018-06-07T17:18:57Z

spec/sql_footprint_spec.rb

+      matches.push 'LIMIT ?'
+
+      matches.each do |match|
+        expect(statements.to_a.detect { |record| record.match match }).not_to be_nil


I stared at this for a while trying to think of a simpler way of stating it, but I've got nothing. Would be nice to simplify, if possible.

mikegee · 2018-06-07T17:20:41Z

@nathantech2005 if you can think of a way to rewrite that one spec to be simpler for the next reader, that would be amazing. Otherwise, wanna squash this into a commit or two? Seems like we have more commits that diff here 😛

less brittle test. clean up 2 rubocop offenses. clean up multi line. clean up multi line. testing older active record version. try upgrading rubocop version. needs rubocop version .40 narrowing gem versions to make more maintainable. ruby v2 uses older active record. add pry back bizarre active record errors. don't specify active record version. try setting minimum gem version. better name for FactoryBoy. ignore jetbrains files using ~/.gitignore_global instead of locally in .gitignore remove two more ignored files. remove debug code

natesire · 2018-06-07T19:46:37Z

I renamed detect to find. They are alias names for same method.

…

On Thu, Jun 7, 2018 at 1:20 PM, Michael Gee ***@***.***> wrote: @nathantech2005 <https://github.com/nathantech2005> if you can think of a way to rewrite that one spec to be simpler for the next reader, that would be amazing. Otherwise, wanna squash this into a commit or two? Seems like we have more commits that diff here 😛 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#34 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEXKPEbQwwPIQQyNQ4e8q1R8UlLViyo8ks5t6WDrgaJpZM4TSihP> .

nathansire added 7 commits April 12, 2018 16:51

fixed test.

9d1a773

remove jetbrains ide settings.

babf599

ignore jetbrains files.

457ce9c

debug writing to file.

50ba5c6

update README. output stored in db dir.

b0ca7b3

FactoryBot is new name for FactoryGirl

657fbfd

remove debug code.

1acd4a8

brandonjoyce suggested changes Apr 13, 2018

View reviewed changes

mikegee reviewed Apr 13, 2018

View reviewed changes

AlexMooney reviewed May 25, 2018

View reviewed changes

mikegee reviewed Jun 6, 2018

View reviewed changes

mikegee reviewed Jun 7, 2018

View reviewed changes

nathansire added 2 commits June 7, 2018 13:50

simplify code on test

2d4a10b

fix rubocop

f794379

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated Rails, Fixed Tests, Updated Readme #34

Updated Rails, Fixed Tests, Updated Readme #34

natesire commented Apr 12, 2018

brandonjoyce left a comment

brandonjoyce Apr 13, 2018

brandonjoyce Apr 13, 2018

brandonjoyce Apr 13, 2018

brandonjoyce commented Apr 13, 2018

natesire commented Apr 13, 2018 via email

brandonjoyce commented Apr 13, 2018

natesire commented Apr 13, 2018 via email

mikegee Apr 13, 2018

natesire commented Apr 13, 2018 via email

natesire commented Apr 14, 2018 via email

AlexMooney May 25, 2018

natesire commented Jun 6, 2018 via email

mikegee Jun 6, 2018

mikegee Jun 6, 2018

natesire commented Jun 6, 2018 via email

mikegee Jun 7, 2018

mikegee commented Jun 7, 2018

natesire commented Jun 7, 2018 via email

Updated Rails, Fixed Tests, Updated Readme #34

Are you sure you want to change the base?

Updated Rails, Fixed Tests, Updated Readme #34

Conversation

natesire commented Apr 12, 2018

brandonjoyce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonjoyce commented Apr 13, 2018

natesire commented Apr 13, 2018 via email

brandonjoyce commented Apr 13, 2018

natesire commented Apr 13, 2018 via email

Choose a reason for hiding this comment

natesire commented Apr 13, 2018 via email

natesire commented Apr 14, 2018 via email

Choose a reason for hiding this comment

natesire commented Jun 6, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natesire commented Jun 6, 2018 via email

Choose a reason for hiding this comment

mikegee commented Jun 7, 2018

natesire commented Jun 7, 2018 via email