-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add tests and fixes for the Collection API #14
Conversation
d397143
to
1613a48
Compare
5bb593d
to
0b62d35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could probably shrink the timeseries data used in the tests, but otherwise this looks good to me.
they were not being inserted anyways
previously, we would rely on `lastrowid` to provide the pk of the inserted row, but since we're using `INSERT OR IGNORE`, we ran the risk of reusing an old `lastrowid` entry. Now, we will only use it if we've confirmed a row was inserted due to the statement.
…omodate test mocking
3d26a90
to
837ba54
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! @cmelone thanks for the quick turn around!
I wrote some tests to handle calls to the
fetch_job
andfetch_node
function. There's a mix of integration and unit tests here.Because a
POST
request to/v1/collect
will return 200 immediately and continue the collection in the background, I decided the best way to perform the tests was directly calling thefetch*
functions.In the course of writing this PR, I also fixed a couple bugs in the collection API