Skip to content

Conversation

@dball
Copy link
Contributor

@dball dball commented Apr 23, 2012

  • Adds test for Quoting module's #quote and #quoted_datetime
    methods when integrated with the adapter
  • Includes test for ActiveSupport::TimeWithZone wrapping Time and DateTime

* Adds test for Quoting module's #quote and #quoted_datetime
  methods when integrated with the adapter
* Includes test for ActiveSupport::TimeWithZone wrapping Time and DateTime
metaskills added a commit that referenced this pull request Apr 24, 2012
Here's the revised patch and semi-integrated test
@metaskills metaskills merged commit e681d6b into rails-sqlserver:master Apr 24, 2012
@metaskills
Copy link
Member

I'm reviewing these now. BTW, I only half read your previous notes.

I don't like the column stubbing. We go to great lengths via our specific schema file to create a real schema for almost any situation and data type. Since getting a real column object is as easy as SomeClass.columns or SomeClass.columns_hash['name'] we really should use those. I may refactor test. I am also gonna yank the should eventuality's. Like TODO's they never get done.

@metaskills
Copy link
Member

I am not sure about tests like these. This looks like it white box testing the framework. Not a fan of that.

should "ask the column class to convert the value" do
  value = "value"
  result = stub("result")
  @column.class.stubs(:string_to_binary).with(value).returns(result)
  assert_equal result, @connection.quote(value, @column)
end

@metaskills
Copy link
Member

I am also finding a few tests failing (https://gist.github.com/2475837) now with this addition. If I comment out the tests you added then they pass. Perhaps it is the way you are doing your setups. I'll see which one, but I suspect using established methods like Time.with_zone {...} may be better, will see.

Copy link
Member

Choose a reason for hiding this comment

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

I will be removing this section. We already have a quote data for sqlserver with literal 0x prefix test in the column test file. BTW, before you say that the tests are spread out all over the place, remember that in many cases we mirror the test setup and organization of how ActiveRecord base is done. Another good example, the test_column_names_are_escaped in base_test_sqlserver.rb file. Either way, removing this.

@metaskills
Copy link
Member

Misc fixes applied in 591a9de

@metaskills
Copy link
Member

A few test fail in 1.9.3 too. I knew this should have been conditional on a ruby version or arity of the method.

metaskills added a commit that referenced this pull request Apr 24, 2012
@dball
Copy link
Contributor Author

dball commented Apr 24, 2012

Huh! I didn't have any test failures in ruby-1.9.3 when last I ran the suite, that's interesting. I'll rerun just to confirm, but your patch is of course fine.

Note I tried to create Column instances, but realize that, when creating a column that had the :datetime AR type, I had to encode a bunch of information that was tightly coupled to inner workings of another method, the stubbing just continued to feel better. But it's of course your call. Most of these tests were more just me getting comfortable anyway, though I do find value in fully testing all the branches of the main quote method.

@metaskills
Copy link
Member

Yea, just remember, the easiest way to get a column instance is to get it from the class. Where base ActiveRecord test cases models don't cover it, we have over a dozen models with many more columns that are just lying around from our test helper. This is where SqlServerChronic comes from and the like.

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

Successfully merging this pull request may close these issues.

2 participants