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

Fix a fixtures test case with table prefix/suffix #4114

Merged
merged 1 commit into from Dec 22, 2011
Merged

Fix a fixtures test case with table prefix/suffix #4114

merged 1 commit into from Dec 22, 2011

Conversation

alexeymuranov
Copy link
Contributor

Make sure the table name of a model is reset in a test case after assigning ActiveRecord::Base.table_name_prefix and ActiveRecord::Base.table_name_suffix. Before this change, Topic.table_name would use the "memoized" value without prefix and suffix.

This change does not currently fix anything, but make the test case to test the right thing. I plan to submit a patch to make fixtures use the table name defined in the associated model, and there this test would fail without this fix.

This was somebody else's test case, so an independent opinion on the change can be helpful.

@josevalim
Copy link
Contributor

/cc @jonleighton

@jonleighton
Copy link
Member

I am not sure about this. Can you rewrite the test to use its own model class rather than modifying Topic?

@alexeymuranov
Copy link
Contributor Author

The problem is that changing ActiveRecord::Base.table_name_prefix will only affect Topic.table_name if Topic.table_name was never called before, otherwise the instance variable @table_name with the old table name will be used. So, i think i am fixing a bug in the test case here.

@alexeymuranov
Copy link
Contributor Author

Do you propose that i do not use an existing fixture/model and add a new one? What difference will it make?

@jonleighton
Copy link
Member

Yes, I am proposing that. Just feels less hackish then this weird way of resetting the table name.

@alexeymuranov
Copy link
Contributor Author

If this means to replace Topic with AnotherTopic, i can do this, but i do not see the difference with the present situation. There will be the same weird way to reset the table name, and somebody might still use AnotherTopic in their tests.

@alexeymuranov
Copy link
Contributor Author

There should be some way to reload the class and reset all class instance variables, but i do not know, maybe you can suggest something along this line?

@alexeymuranov
Copy link
Contributor Author

Ok, i think i've understood what you meant: that the fixture/model be unique in the FixturesTest class.

@alexeymuranov
Copy link
Contributor Author

How about this? But i do not understand very well what this test is doing, i was just fixing a small bug.

@jonleighton
Copy link
Member

Yes, that looks good, but you could actually just create the class instance within the test, which will ensure that it is definitely not used elsewhere:

[1] pry(main)> require 'active_record'
=> true
[2] pry(main)> klass = Class.new(ActiveRecord::Base) do
[2] pry(main)*   def self.name
[2] pry(main)*     "Topic"
[2] pry(main)*   end  
[2] pry(main)* end  
=> #<ActiveRecord::Aggregations::ClassMethods:0x10f0810>
[3] pry(main)> klass
=> #<ActiveRecord::Aggregations::ClassMethods:0x10f0810>
[4] pry(main)> klass.table_name
=> "topics"

@alexeymuranov
Copy link
Contributor Author

Ok, i'll try to do like this. I still think to use the same commit to order the require at the top alphabetically.

Make sure the table name of a model is reset in a test case after assigning ActiveRecord::Base.table_name_prefix and ActiveRecord::Base.table_name_suffix.  This was somebody else's test case, so an independent opinion on the change can be helpful.
@alexeymuranov
Copy link
Contributor Author

I think i've finished.

require 'models/task'
require 'models/topic'
require 'models/traffic_light'
require 'models/treasure'
require 'tempfile'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorted alphabetically.

jonleighton added a commit that referenced this pull request Dec 22, 2011
…fixtures_test

Fix a fixtures test case with table prefix/suffix
@jonleighton jonleighton merged commit f8e484d into rails:master Dec 22, 2011
jonleighton added a commit that referenced this pull request Dec 22, 2011
…_suffix_fixtures_test"

This reverts commit f8e484d, reversing
changes made to fa5adfb.

Reason: broke the postgres tests.
@jonleighton
Copy link
Member

This broke the postgres tests: http://travis-ci.org/#!/rails/rails/jobs/437615

I have reverted - please submit a new PR with a fix for that. Thanks.

@alexeymuranov
Copy link
Contributor Author

I do not know when i will have time to look into it, i haven't figured out how to use PostgreSQL on my Mac OS.

All errors are in the same line where database table rows are first used. It looks like the table must have been empty, but strange that this only happens with PostgreSQL. The only thing that i've changed that i can think of is that i do not have fixture :other_topics anymore, but isn't create_fixtures("other_topics") supposed to load the fixture data into the table?

@alexeymuranov
Copy link
Contributor Author

I've installed PostgreSQL! I'll try to debug this.

@alexeymuranov
Copy link
Contributor Author

How do i run a single test with postgresql?

@jonleighton
Copy link
Member

ARCONN=postgresql ruby -Itest path/to/test.rb

@alexeymuranov
Copy link
Contributor Author

Thanks!

@alexeymuranov
Copy link
Contributor Author

I have no idea what it was. I went back to master and started slowly applying patches. I created OtherTopic model identical to Topic, added table creation to schema.rb, did other things, then removed them, and i ended up with my original commit that now passes the postgresql test, but was not passing it before. What should i do?

@alexeymuranov
Copy link
Contributor Author

$ bundle exec rake test gave me

...
3302 tests, 10188 assertions, 0 failures, 0 errors, 10 skips

@alexeymuranov
Copy link
Contributor Author

Could have something to do with the state of the test database?

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.

None yet

3 participants