Use model class DB connection in have_db_index matcher #1200
Conversation
| @@ -3,7 +3,8 @@ | |||
| describe Shoulda::Matchers::ActiveRecord::HaveDbIndexMatcher, type: :model do | |||
| context 'have_db_index' do | |||
| it 'accepts an existing index' do | |||
| expect(with_index_on(:age)).to have_db_index(:age) | |||
| expect(with_index_on(:age1)).to have_db_index(:age1) | |||
| expect(with_index_on(:age2, parent_class: ProductionRecord)).to have_db_index(:age2) | |||
There was a problem hiding this comment.
Hey sorry it took me so long to get back to you. Very nice solution overall to this. I think the only thing I'd say here is that it might be beneficial to be a little more explicit in this particular case. It might be lost on the reader that ActiveRecord::Base is being used by default for the first assertion here and that it defaults to using the development database. Perhaps for symmetry's sake it would be better to also create a DevelopmentRecord (which would essentially do nothing) and then be explicit about that here:
expect(with_index_on(:age1, parent_class: DevelopmentRecord)).to have_db_index(:age1)
expect(with_index_on(:age2, parent_class: ProductionRecord)).to have_db_index(:age2)There was a problem hiding this comment.
That makes sense. I have updated accordingly.
* In current database.yml config, all database environments are sharing same database. Suffix database name with relevant environment name to ensure database uniqueness * Update Rails migration command to `db:drop:all` and `db:create:all`, so all db listed in config/database.yml is dropped/created accordingly * Write a base ActiveRecord model that connects to different database (production database) * Write a base ActiveRecord model that connects to default database (development database), this is a dummy model, just for symmetry's sake
* create_table and define_model_class accepts optional `connection' parameter for custom db connections * clear_column_caches and drop_created_tables for ProductionRecord
|
There're still some failing specs. I'll look into those over the weekend. |
|
@jooeycheng I looked into this a bit for you. I think the problem is that both of the models you're creating in that test have the same class name. The method that creates classes will (unhelpfully, now, I realize) detect whether the class it's creating already exists and remove it before adding its version. After the tests are run, it will then make sure that class is removed. So the second model is overwriting the first model, however, when it goes to remove that class after the test is finished running, it gets very confused. If you give |
|
@mcmire I just noticed you pushed this commit e716a5b. I think it's structured better, with clearer naming. I have updated to follow it. |
|
@jooeycheng Oops, I noticed you updated this just now. Skimming over it again I think it looks good, but I'll take a closer look when I get a chance. |
|
Thanks so much for this! |
|
@jooeycheng Yeah there's definitely some complex parts to the gem that not many people know about but I'm glad I was able to help you navigate them. If you find something else let me know and I will happily take another PR of yours :) |
Fix #1051. This PR is WIP.
Hey @mcmire, lemme know what you think.
I'm thinking to rename
ProductionRecordto something else, as this naming might cause confusion.