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

MySQL test success depends on using InnoDB table type #6

Closed
jblebrun opened this Issue May 15, 2013 · 9 comments

Comments

Projects
None yet
2 participants
@jblebrun
Contributor

jblebrun commented May 15, 2013

In MySQL, InnoDB tables will increment the 'id' counter when inserts fail, but MyISAM tables do not. So, if the default settings for MySQL are to use MyISAM, a number of the select tests will fail, since they compare the ID.

I'm not sure what the best way to approach this would be... I only thought of the following three, but not sure which would be preferred:

  1. Adding documentation in the tests directory mentioning the InnoDB requirement
  2. Removing ID checks in tests
  3. Adding a command specifying engine when creating tables in MySQL

@tgriesser tgriesser closed this in 469d600 May 15, 2013

@tgriesser

This comment has been minimized.

Owner

tgriesser commented May 15, 2013

@jblebrun - thanks for the heads up... I just pushed some changes to be able to add the engine on createTable... let me know if this looks good.

@jblebrun

This comment has been minimized.

Contributor

jblebrun commented May 16, 2013

This does the trick, although it does break the abstraction a bit. Since only MySQL bindings support the engine function for now, it's not a huge deal, but it might be worth thinking through how to cleanly separate implementation-specific details further down the road.

@tgriesser

This comment has been minimized.

Owner

tgriesser commented May 16, 2013

Well, it's a no-op if used when it's not a MySQL connection, since it's only used in the compileCreateTable for the MySQL builder... Do you think that'd be alright?

@tgriesser

This comment has been minimized.

Owner

tgriesser commented May 16, 2013

@jblebrun - The other I was thinking is maybe if the engine type could be set in the config settings in Knex.Initialize? Then it wouldn't be per-table though...

@jblebrun

This comment has been minimized.

Contributor

jblebrun commented May 16, 2013

I think doing it in the Initialize step would be better. Otherwise implementation details start to leak into what is supposed to be an implementation-independent means of specifying a schema.

@jblebrun

This comment has been minimized.

Contributor

jblebrun commented May 16, 2013

For example, what if a database is later added that also supports engines, but different kinds? You couldn't simply change the initializer and expect things to work - the new database might break when it encounters the unexpected engine name that was intended for MySQL.

That said, it's pretty hard to write a schema that is truly implementation independent, if you expect to use all of the features of the database. However, keeping a separation between implementation-specific details and common schema detail makes it easier to migrate from one platform to another - it's easy to detect which parts need to be changed and which don't.

@jblebrun

This comment has been minimized.

Contributor

jblebrun commented May 16, 2013

As an aside, your latest change breaks the tests, since the sql is expected to contain the "engine = " for the mysql statements.

@tgriesser

This comment has been minimized.

Owner

tgriesser commented May 16, 2013

The engine should be set in the tests in the latest...

I agree with the point that you might not be able to simply change the Initializer, though I feel like ultimately the engine should be set on a per-table basis... Also, InnoDB is the default engine with MySQL so there shouldn't normally be a need specify this anyway. I think I'm going to keep it per-table for now and re-visit if there's any conflict down the line.

@jblebrun

This comment has been minimized.

Contributor

jblebrun commented May 16, 2013

I see, the problem was that the key name in the config file for charset encoding changed from "encoding" to "charset". Since I'm using a different config file, I didn't get this change.

tgriesser added a commit that referenced this issue May 18, 2013

Merge branch 'master' into gh-pages
* master:
  fixing issues with transactions
  tests explicitly specifying InnoDB, fixes #6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment