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

Reset column info after making Topic tz-aware #35271

Merged
merged 1 commit into from Feb 19, 2019

Conversation

gmcgibbon
Copy link
Member

Fixes #35194.

This is a really strange bug.

For some reason, .reload_schema_from_cache called by Topic.reset_column_information can change how date time columns behave. It seems to change included modules of ActiveRecord::Type::DateTime instances, but only sometimes.

In the expected flow, the value cast starts here and includes [ActiveRecord::Type::Internal::Timezone, #<ActiveModel::Type::Helpers::AcceptsMultiparameterTime:0x00007f8cae260358>, ActiveModel::Type::Helpers::TimeValue, ActiveSupport::ToJsonWithActiveSupportEncoder, ActiveSupport::Dependencies::Loadable, JSON::Ext::Generator::GeneratorMethods::Object, ActiveSupport::Tryable, Kernel]. In the failing test case, the value cast starts here and includes [ActiveSupport::Tryable, #<Module:0x00007f8b010f1e58>].

This seems to be triggered by initializing a new topic in AttributeMethodsTest#test_YAML_dumping_a_record_with_time_zone-aware_attribute after resetting column info in a previous test. My proposed solution is more of a band-aid to minimize the blast radius of attribute changes between tests (this was already being done for other tests in AttributeMethodsTest).

@gmcgibbon
Copy link
Member Author

gmcgibbon commented Feb 14, 2019

This seems to break other time related tests strangely, I will continue to investigate.

I should mention too that this is not specific to MySQL. You can reproduce the problem with any adapter:

bin/test test/cases/serialized_attribute_test.rb test/cases/attribute_methods_test.rb test/cases/base_test.rb -n "/^(?:SerializedAttributeTest#(?:test_serialize_does_not_eagerly_load_columns)|AttributeMethodsTest#(?:test_YAML_dumping_a_record_with_time_zone-aware_attribute)|BasicsTest#(?:test_preserving_time_objects_with_time_with_zone_conversion_to_default_timezone_local))$/" --seed 17411 -v

@gmcgibbon gmcgibbon force-pushed the fix_time_attribute_test_failures branch from c72f3cc to a3594d3 Compare February 18, 2019 22:54
In AttributeMethodsTest, we make the global Topic class time zone-aware
which changes instance date time attribute casting behaviour. We need to
reset column info after the test because future tests don't expect Topic
date time columns to be time zone-aware.
@gmcgibbon gmcgibbon force-pushed the fix_time_attribute_test_failures branch from a3594d3 to 08e78ad Compare February 18, 2019 22:56
@gmcgibbon
Copy link
Member Author

It turns out this line initializes a Topic with time_zone_aware_attributes enabled, which causes the date time attribute differences. We need to use a global class here because of how YAML dumping/loading works (other tz-aware column tests don't use the global Topic class). I've opted to simply reset column info on Topic after the test to tell Active Record to forget about the attribute change.

@gmcgibbon gmcgibbon changed the title Use copy of Topic for attributes test Reset column info after making Topic tz-aware Feb 18, 2019
@gmcgibbon gmcgibbon merged commit 16a8072 into rails:master Feb 19, 2019
@gmcgibbon gmcgibbon deleted the fix_time_attribute_test_failures branch February 19, 2019 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants