-
Notifications
You must be signed in to change notification settings - Fork 84
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
Small tests refactoring and added custom exceptions #37
Conversation
Thank you for the PR! I'm traveling this weekend, but I'll make some time when I get back to do a full vetting of what you've got here. I just wanted to chime in quick and let you know I'm not ignoring your work! |
Excellent work! I'm really, really happy with this PR. The one bit that nags at me is the skipped test. I think I know why it's being skipped, but maybe it ought to be moved into one of the PG-specific tests, instead of simply being skipped? Or am I misunderstanding why you're skipping it? |
I am skipping it because your test suite is against a SQLite dummy app in the tests and it won't be able to execute successfully this test. The idea of introducing a CI in the previous pr is to keep the SQLite general tests on the normal CI process (maybe testing against a few ruby version) and to create a docker container for postgresql (and one for MySQL) to run the adapter specific tests within it. In this way you won't have to mock anything! (E.g. https://docs.docker.com/compose/rails/ ) I will think about an alternative way to perform this commented test within the normal test suite in the meantime 😄 |
9b3a86b
to
72ccc1c
Compare
after 8985aee the build is broken but something very interesting is happening. When you run the tests against PG instead of SQLite the FALSE are returned and 0, TRUE as 1 and a few other differences. => We cannot just run the same test suite against all adapters out of the box. I did an utility script run in travis for wrapping what we need to prepare every adapter test execution. Assuming you have a running postgresql server running locally you can just launch:
|
Hey @jamis , do you have any feedback on this pr? I can eventually add tests against the mysql adapter and it would be finished |
@mberlanda I'm so sorry for dropping the ball here! Yes, I really like the direction you're going with this PR; I've wanted to have separate tests for the major DB's and haven't found the time to make that happen, so this really pleases me. I'm finding that I have very little opportunity to use this gem myself---the use-case that spawned it kind of dried up for me some time ago. As a result, my motivation to maintain it has plummeted. I've invited you to be a collaborator on this project, since you seem to have a good vision for the project. If you're okay with being a committer here, feel free to accept the invitation, but I please don't feel any obligation. :) Thanks for your work on this project, either way! |
Hey Jamis, I just accepted your invitation. Thank you very much. This look perfect to me I will then add tests against MySQL adapter and then I would consider this pr ready. |
I came up with a different approach in
|
Hey @jamis ,
in this PR I did a little bit a few things:
mock_worker
factory (which will be able to pass the adapter) and splitting the tests by adapter family (mysql, pg,sqlite)primary_key_return_statement
by adapterBulkInsert ::OptionsNotAvailable
to provide some argument validation based on adaptersThis last point is very interesting to ensure that some side-effects won't be considered as features 😄 In particular it will allow to make sure that
:ignore
and: return_primary_keys
will be provided only for supported adapters.