Code to fix too long table named#45
Conversation
berlincount
force-pushed
the
shortened_failed_lhm_table_names
branch
from
2309215 to
1a48842
Compare
| def failed_name | ||
| "lhma_#{Timestamp.new(Time.now)}_#{origin_table_name}_failed" | ||
| # NOTE: MySQL table names are limited to 64 characters | ||
| "lhma_#{Timestamp.new(Time.now)}_#{origin_table_name}_failed"[0..63] |
There was a problem hiding this comment.
think we should also strip the ms, and probably even the underscores from the timestamp?
There was a problem hiding this comment.
I'd like to see the underscores and ms go as well.
|
@bbuchalter with the microseconds removed some renames started failing in https://buildkite.com/shopify/lhm/builds/69#65b3eea9-4ee7-473c-825c-a9eaf267d9fb .. do you think I might have hit a bug previously hidden by them? |
My initial hypothesis is that the integration test suite is not "cleaning" the database between test runs. It was relying on this precision in timestamp to avoid duplicate table names, and the removal of it has exposed this flaw in the design of the tests. I'll poke at it a bit and see if we can't make the world a better place, as I do agree with your change. |
|
heh, well that makes sense count. in production, there won't be two cleanups running within the same second. in the test environment, that's likely to happen. we probably need to properly clean up the state in between tests. |
|
Does: Need an |
|
|
||
| module Lhm | ||
| VERSION = '3.1.0' | ||
| VERSION = '3.1.1' |
There was a problem hiding this comment.
@berlincount I try to bump versions in isolated commits. Makes it easier to identify what changed between specific releases. Also, you need to include Gemfile.lock changes that accompany this change. See https://github.com/Shopify/lhm#releasing-new-versions
| describe 'failed_name' do | ||
| it 'should provide a table name with a maximum length of 64' do | ||
| cleanup = Lhm::Cleanup::Current.new(nil, 'some_excessively_long_table_name_oh_dear_code_who_would_even_do_this_and_why', nil) | ||
| assert_equal cleanup.instance_eval { failed_name }.size, 64 |
There was a problem hiding this comment.
instance_eval this is an anti-pattern which we should not tolerate. If the code is not easily testable, the code needs to change.
There was a problem hiding this comment.
Can you explain why instance_eval a bad idea? Or at least give me a hint why so I can google up an article to help me understand.
There was a problem hiding this comment.
instance_eval is a way to "reach inside" an instance of a class and do whatever you wish. You can manipulate the behavior of the instance or query it's internals. It's a powerful, but dangerous tool. In this particular context, we're using it to make assertions about the internals of cleanup. This suggests that what we want to test isn't publicly available which suggests that the code responsible for this needs to be extracted into it's own class, which can be tested via normal means. Happy to dig into this further if you have other questions. Thanks for asking!
This is a super anti-pattern. We should not be using code under test to manage the state of our tests! I'm going to replace this with the defacto standard for database cleanup between test runs: https://github.com/DatabaseCleaner/database_cleaner. |
Isn't that similar to adding code to a That |
|
Well just fix it (tm) ;-P |
|
(sorry if I'm being overly flippant, but .. I just/only stepped on that landmine 🙄 ) |
|
Before we attempt to address the timestamp problem, let's separately address the test-state-leaks we discovered here: #46. Since this PR will depend on that work, let's just close it for now. |
|
The LHM stays unrunable without this PR (or similar) due to broken cleanup :( |
|
This PR is replaced by #49 |
4 participants
MySQL has a maximum table name length limit of 64 bytes, so we need to make sure to not generate table names longer than this. The table names are still guaranteed to be unique, as we have usec-precision Timestamps in them.