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

Integration test #61

Merged
merged 4 commits into from
Sep 30, 2016
Merged

Integration test #61

merged 4 commits into from
Sep 30, 2016

Conversation

Antar1011
Copy link
Owner

In writing this test, I've discovered a major bug owing to how SQLAlchemy compiles queries. Basically, the current scheme will not work for a full species-lookup. It also won't handle bulk inserts.

See: http://stackoverflow.com/questions/7106016/too-many-sql-variables-error-in-django-witih-sqlite3

The bulk inserts issue can be resolved by doing them the correct way (that is, using the values argument in execute() rather than the .values() function in Insert).

The solution to the species-lookup issue appears to be making a temporary table.

@Antar1011
Copy link
Owner Author

This build will fail, but it's because this PR has revealed a bug, not because it's introduced one, so I'm merging it, even as I work on the solution.

@Antar1011 Antar1011 merged commit b7ad511 into master Sep 30, 2016
@coveralls
Copy link

coveralls commented Sep 30, 2016

Coverage Status

Coverage remained the same at 99.71% when pulling c7cc220 on integration-test into 3827b0e on master.

@Antar1011
Copy link
Owner Author

To be clear, I don't love that this test is randomly generated. But I did it with the LogProcessor tests as well, so if I'm going to make an issue of it here, I should fix it there too. Maybe I'll make a "tech debt" ticket?

@Antar1011 Antar1011 mentioned this pull request Sep 30, 2016
@Antar1011
Copy link
Owner Author

Antar1011 commented Sep 30, 2016

Weirdly, the error I reported in #62 is not the error that gets thrown on Travis. Wonder if the difference is due to different versions of SQLAlchemy..?

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.

2 participants