Change the message string to use in test case. #5306

Merged
merged 1 commit into from Mar 8, 2012

Projects

None yet

4 participants

@kennyj
Contributor
kennyj commented Mar 6, 2012

I think we should have been originally as this PR when testing quoted database.

Please see also #5291

@carlosantoniodasilva

Hey @kennyj, I'm a little bit worried this may make the test more brittle. Lets say tomorrow mysql changes the error message, we'll never know because the test will continue passing - I know it's unlikely to happen, but who knows.

Also, I think that testing the message below already gives you certainty that any syntax error happened, and that the database name was properly quoted in the executed sql.

`Unknown database 'foo-bar': SHOW TABLES IN `foo-bar`

What do you think?
@rafaelfranca thoughs?

@rafaelfranca
Member

But when the user don't have access to the database the message will be:

mysql> show tables in `foo-bar`;
ERROR 1044 (42000): Access denied for user ''@'localhost' to database 'foo-bar'

So if we want to change the message exception we need to give access to the rails user.

Maybe, this should work:

assert_match(/database 'foo-bar'/, e.inspect)
@kennyj
Contributor
kennyj commented Mar 7, 2012

Hey Bros.

Umm... I had the same mesasge as @rafaelfranca, but it seems that others had the another message.

I hope: I don't want to execute "GRANT ALL PRIVILEGES ON foo-bar.* to 'rails'@'localhost';" because I think making this setting only for this test is cumbersome.

But

I'm a little bit worried this may make the test more brittle. Lets say tomorrow mysql changes the error message, we'll never know because the test will continue passing - I know it's unlikely to happen, but who knows.

Agree with an above mention.

I want use exactly and short sentence.
Thus, +1 @rafaelfranca's plan :)

assert_match(/database 'foo-bar'/, e.inspect)
@carlosantoniodasilva

Yeah, I agree with @rafaelfranca as well.. I was not running the tests with a rails user, so I didn't get this error, and neither travis did.. not good having to add this GRANT, so 👍

@kennyj
Contributor
kennyj commented Mar 7, 2012

@rafaelfranca could you send pr ?, or I fix and rebase ?

@rafaelfranca
Member

@kennyj please. Go ahead

@kennyj
Contributor
kennyj commented Mar 7, 2012

updated :)
Thanks guys !

/cc @josevalim

@kennyj
Contributor
kennyj commented Mar 8, 2012

If we don't merge this PR, somebody happen the following error.

Finished tests in 103.945188s, 32.0746 tests/s, 98.3307 assertions/s.

  1) Failure:
test_tables_quoting(ActiveRecord::ConnectionAdapters::Mysql2SchemaTest) [/home/kennyj/rails/activerecord/test/cases/adapters/mysql2/schema_test.rb:45]:
Expected /Unknown database 'foo-bar': SHOW TABLES IN `foo-bar`/ to match "#<ActiveRecord::StatementInvalid: Mysql2::Error: Access denied for user 'rails'@'localhost' to database 'foo-bar': SHOW TABLES IN `foo-bar` >".

3334 tests, 10221 assertions, 1 failures, 0 errors, 5 skips
@josevalim josevalim merged commit 58a5559 into rails:master Mar 8, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment