Rails AR/SQLServer Unit Test: [7909] failed (getting worse)

"bitsweat" has kicked AR/SQLServer while it was down...

http://dev.rubyonrails.org/changeset/7909

Hi All,

Is there a specific reason why all those scripts in
activerecord/test/fixtures/db_definitions exist instead of one database
independent migration file?

Imo the ability or inability to get the migration file correctly
imported in the database of your choice is already a test.

Regards,

Lawrence

+1 for schema in Ruby format … I don’t think that tests use db features that AR::Migrations don’t support currently.

As you can use execute in migrations, there should be the possibility of including tests for database-specific implementation. I don't know if there are any such tests right now, but unless the current situation is actually broken, I don't see a reason why to fix it.

Michael Glaesemann
grzm seespotcode net

The current situation is not broken, but difficult to maintain as schema changes require multiple files to be updated with different syntaxes. AR::Migrations were created to solve exactly this problem, and I think they’re powerful enough to set up the database for testing.

By the time migrations were added, there were a lot of tests using
those old schema files. Since then, all future additions should have
been made in the ruby schema files. David has asked several times for
someone to clean that up and convert to ruby schema files, but it is a
larger job than it may initially look. Does someone want to give it a
shot?

All those sql files under db_definitions need to be moved to
schema.rb, right?

Does it have to be in 1 patch? Maybe we can do this 1 db adapter at a
time, and initially just add a condition to check for the adapter in
schema.rb.

Is there a specific reason why all those scripts in
activerecord/test/fixtures/db_definitions exist instead of
one database independent migration file?

Largely because they were created before migrations and the schema dumper.

There are also some db definitions that can't be done in schema.rb.

Imo the ability or inability to get the migration file correctly imported
in the database of your choice is already a test.

All new additions use schema.rb in the same dir.

jeremy

Other issues include:

  1. Inconsistencies between different databases: for example
    topics.content is of type VARCHAR(255) in SQL Server but of type TEXT
    in MySQL.

  2. If any .sql files should exist, then they should exist within the
    plugin adapter, not within rails core.

I’ll have a shot at transforming this into a migrations file. I’ll
attempt this first for SQL Server only, this should not affect core. If
that works we can see if/how this can used by all the other adapters.

There are also some db definitions that can’t be done in schema.rb.

Ok, I see those. We should be able to work around 'm… I’ll have a look.

Regards,

Lawrence

All those sql files under db_definitions need to be moved to
schema.rb, right?

Does it have to be in 1 patch? Maybe we can do this 1 db adapter at a
time, and initially just add a condition to check for the adapter in
schema.rb.

I think that'd be fine. If it's a temporary condition put in place
until the transition is done, just keep it commented so it's obvious
what's going on. Smaller patches are easier to deal with.

I've encountered some problems with simply moving the sql files to
schema.rb. The sql files that end with 2 need to connect to
activerecord_unittest2. Moving it to schema.rb would result to having
the courses table created in activerecord_unittest1.

Is it OK to create a schema2.rb to be used for the courses table? That
is, in AAACreateTablesTest#test_drop_and_create_courses_table, we can
read schema2.rb in the same way that schema.rb is read in
AAACreateTablesTest#test_load_schema.

See patch

Cheers,

Lawrence

Thanks for the link. I kinda liked the approach of doing this 1 db
adapter at a time since I don't want to send a patch that affects
other databases that I can't test locally. Anyway, I'll submit a patch
soon as I'm almost done moving db/definitions/mysql*.sql to
schema*.rb. I'll keep a close watch on your ticket's progress. I think
I'll just have 2 patches in my new ticket, 1 for mysql.sql and the
other for mysql2.sql so that in case your patch is accepted, we can
just ignore the mysql2 patch. What do you think?

Btw, is anyone also working on a patch for MySQL? I'm just checking
coz I'll stop my work on this if anyone already is. I plan to do this
for PostgeSQL after.

Added

http://dev.rubyonrails.org/ticket/9899

FYI

> since I don’t want to send a patch that affects
other databases

Note that when one adds a test, this already affects all
databases. I’m pretty sure tests and patches get into core without
being tested on all or at least a significant number of databases. So
this is really a general issue, not just for this little project.

The problem with doing this one adapter at a time is that if you
created a schema.rb based on mysql.sql and someone else creates a
schema.rb based on say oracle.sql or postgresql.sql, you’d end up with
significant different schema.rb files. So whose schema.rb is going to
end up in core?

I had also moved all mysql.sql statement into a schema.rb (except for
tables subscribers, fk_test_has_pk and fk_test_has_fk, for which no
migrations equivalent exists). Works fine for mysql, but then shows
quite a few regressions on quite a few other databases. For example,
posts.body is of type text in mysql, so you’d think t.text :body would
be the answer, unfortunately the tests do a search via that field which
is not an allowed operation on e.g. SQL Server (which is why in the sql
stament SQL Server defines the field as varchar(4096) instead of text;
same for Oracle, Sybase and I suspect Firebird and DB2 as well).

I guess the one db adapter at a time approach works, but imo we need to
do this sequentially. So if you create a patch for mysql, then we all
should first get behind that one and get it into core before we move on
to the next database adapter.

Lawrence

> since I don't want to send a patch that affects other databases

Note that when one adds a test, this already affects all databases. I'm
pretty sure tests and patches get into core without being tested on all
or at least a significant number of databases. So this is really a
general issue, not just for this little project.

I actually ended up changing AAACreateTablesTest and I realized what
you mean (that I've just added a patch that affects other databases),
so I guess that wasn't the right argument. For my patch though, it
surely won't work for other databases without the condition that I
added to check if the adapter is MySQL. I guess what I meant is that
since I'm not very familiar with the other database adapters, I'm not
capable of creating a patch for this particular issue that would work
for all. So I was hoping we could do this with the help of other
contributors familiar with the other databases.

Btw, instead of creating 2 patches as I've mentioned before, I didn't
create the patch for removing mysql2.sql and mysql2.drop.sql. Instead,
I just referenced your ticket so there's no wasted effort.

I guess the one db adapter at a time approach works, but imo we need to
do this sequentially. So if you create a patch for mysql, then we all
should first get behind that one and get it into core before we move on
to the next database adapter.

It would be great if we could do this sequentially.

I’m testing your patch. Works fine for MySQL, of course :wink:

Nice solution for table subscribers.

While currently schema.rb begins with a line to test that the adapter
is MySQL, the end goal is to remove that condition. So all databases
would use the exact same migrations code.

Take e.g. table :tasks. It defines the date fields as not null with a
default. However, all other database sql files define the fields as
nullable and no default. If we accept this patch, then for the next db
adapter this would need to change. I.e.: it would affect all adapters
that so far had switched to migrations.

Other things include:

  • Remove :options => ‘TYPE=InnoDB DEFAULT CHARSET=utf8’. It will
    then work for all databases, and still works for MySQL as well (if you
    have created your database using charset UTF8)

  • Explicitly define :scale => 0 for :numeric_data.world_population
    and my_house_population

  • We can, for now, change all columns of type :text to type :string. It
    should then work for other databases as well. However, it then does add
    one failing test for MySQL, but that is a test specific to MySQL only
    (i.e., modify the test to use a new table with a column of type :text).

  • The statement to create the foreign key should probably go into a
    seperate file for use by MySQL only. I.e., some statements will stay to
    be adapter specific.

Doing the above I got it reasonably well working for SQL Server as
well. :slight_smile:

Generally I would have prefered the sexy migrations syntax, but that’s
a different issue :slight_smile:

And some more:

  • You forgot to include the patch for the courses table. :wink:

  • In the rakefile, target mysql:build_databases, remove the statements
    that would import the sql files.

Btw, I am giving a +1 to the patch in its current state,
as it
does work for MySQL, which was the goal for this patch. The above can
be modified later.

Happy to have this train moving :slight_smile:

Cheers,

Lawrence

Exactly the kind of feedback I was hoping for. :slight_smile:

I'll apply most if not all of your comments. I'll have a closer look
later after work, and expect an updated patch in maybe 10 hours.

Thanks a lot!

New patches have been added to the ticket. FYI

http://dev.rubyonrails.org/ticket/9899

Hi All,

As a continuation to migrate more database adapters towards use of
schema.rb instead of .sql files to create the test environment:

Need 2 more +s please.
In case you wonder why the sqlite_adapter needs a change: if you keep
it as it is today, then the .sql files create the topics table with
column bonus_time of type “TIME”. However, if you create the topics
table via a migrations file, using “t.time bonus_time”, then currently
it will be created with type “DATETIME”. Running the tests sqlite3 will
then show these two failures:

  1. Failure:
    test_attributes_on_dummy_time(BasicsTest) [test/base_test.rb:959]:
    <Sat Jan 01 05:42:00 +1100 2000> expected but was
    .
  2. Failure:
    test_utc_as_time_zone(BasicsTest) [test/base_test.rb:688]:
    <Sat Jan 01 05:42:00 UTC 2000> expected but was
    .
    This is because now with type “DATETIME”, in schema_definitions.rb
    method type_cast(value) the :datetime type will go through method
    string_to_time. Instead, with type “TIME” is would go through the
    method string_to_dummy_time which is what it should do.
    After this change I propose the Postgresql adapter also starts using
    the schema.rb file.
    Cheers,
    Lawrence