Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Deprecation removed build fixed #11274

Merged
merged 1 commit into from Jul 7, 2013

Conversation

Projects
None yet
5 participants
Member

arunagw commented Jul 3, 2013

No description provided.

@arunagw arunagw and 4 others commented on an outdated diff Jul 3, 2013

...record/test/cases/migration/column_attributes_test.rb
@@ -177,7 +177,7 @@ def test_native_types
with_env_tz 'US/Eastern' do
bob.reload
- assert_equal DateTime.local_offset, bob.moment_of_truth.offset
+ assert_equal DateTime.civil_from_format(:local, 2012), bob.moment_of_truth.offset
@arunagw

arunagw Jul 3, 2013

Member

I am not sure about this test. Might fail on oracle but we need to place something as local_offset is removed.

I will test this using oracle adaptor.

Or may be @yahonda want's to have a look

@pixeltrix

pixeltrix Jul 3, 2013

Owner

If you look a couple of lines above there's this:

# Test DateTime column and defaults, including timezone.
# FIXME: moment of truth may be Time on 64-bit platforms.
if bob.moment_of_truth.is_a?(DateTime)

Which dates back to db69c9c when we used to fallback to using DateTime when the range of valid Time values was limited. This is no longer the case for Rails 4.0+ - it should always be a Time so I doubt even if this block of code is being tested.

In fact If it is returning a DateTime, I'd consider that a bug 😄

@yahonda

yahonda Jul 3, 2013

Contributor

Just cherry-pick f51ddc39637bc259bf4293a9c90622fe63f39e3c to master and tested with Oracle enhanced adapter, it passes successfully.

$ ARCONN=oracle ruby -Itest test/cases/migration/column_attributes_test.rb -n test_native_types
Using oracle
Run options: -n test_native_types --seed 31832

# Running:

.

Finished in 0.215049s, 4.6501 runs/s, 65.1015 assertions/s.

1 runs, 14 assertions, 0 failures, 0 errors, 0 skips
$
@pixeltrix

pixeltrix Jul 4, 2013

Owner

@yahonda look at the assertion count - it's 14 which means that the changes in f51dd3 aren't tested otherwise the assertion count would be 20.

@arunagw the block of code is scoped with unless current_adapter?(:OracleAdapter) so it never runs on Oracle anyway.

@arunagw

arunagw Jul 4, 2013

Member

Ahh true. It's unless thanks @pixeltrix

I wil check back this PR and update accordingly.

@arunagw

arunagw Jul 4, 2013

Member

I just checked and found that this code is not being called.

Do we need to update this test? or just simply remove this block of code?

Because there is no method called .offset

@rafaelfranca

rafaelfranca Jul 7, 2013

Owner

It is safer to remove in my opinion

@arunagw arunagw commented on an outdated diff Jul 3, 2013

activesupport/test/core_ext/module_test.rb
@@ -278,12 +278,6 @@ def test_parents
def test_local_constants
assert_equal %w(Constant1 Constant3), Ab.local_constants.sort.map(&:to_s)
end
-
- def test_local_constant_names
@arunagw

arunagw Jul 3, 2013

Member

This is fine.

Member

arunagw commented Jul 7, 2013

PR updated.

thanks.

rafaelfranca added a commit that referenced this pull request Jul 7, 2013

@rafaelfranca rafaelfranca merged commit 7d384dd into rails:master Jul 7, 2013

1 check passed

default The Travis CI build passed
Details

@arunagw arunagw deleted the arunagw:deprecation-removed-build-fixed branch Jul 9, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment