-
Notifications
You must be signed in to change notification settings - Fork 67
Add basic integration tests for all endpoints + API Keys tests #1241
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 basic integration tests for all endpoints + API Keys tests #1241
Conversation
SonarCloud Quality Gate failed.
|
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.
this is awesome! You put a lot of work into this PR, and it will be very much worth the time when these tests help someone catch a bug later!
from delphi.epidata.server._limiter import limiter | ||
|
||
|
||
class BasicIntegrationTest(unittest.TestCase): |
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.
class BasicIntegrationTest(unittest.TestCase): | |
class IntegrationTestBase(unittest.TestCase): |
^ name suggestion to hopefully make it obvious that this is a "base" class. (im also a little bit tempted to call it DelphiTestBase
, but thats up to you)
this class has a number of things in common with our other unittest.TestCase
subclass / test base class helper: CovidcastBase
. CovidcastBase
would best be redesigned as a subclass of the common IntegrationTestBase
that then adds epimetric_*
table truncations in setUp() and the additional row helper methods.
would it be difficult to combine them?
NOTE: maybe the suggested changes of combining / refactoring with CovidcastBase
could be its own PR (if at all) after this one is merged. also, CovidcastBase
has a number of things i think should change, such as changing the name to CovidcastTestBase
and relocating the file to src/common/
instead of being under src/acquisition/
.
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.
Yeah, that renaming is reasonable.
Yeah, I have also thought about combining this class with CovidcastBase, but it will be better to do that in a separate PR(after this one is merged).
self.delete_from_tables_list = [] | ||
self.truncate_tables_list = [] |
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.
do we need both DELETE
and TRUNCATE
? TRUNCATE
is faster for bigger tables, but that shouldnt really matter in integration tests. TRUNCATE
will also reset AUTO_INCREMENT
counters, but hopefully all of our tests are agnostic to any assigned id
values and such.
it will be simpler to have only one of these, but if there is a good reason for having both, thats fine too.
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.
That is unfortunate but yes, we need both of them. There are some tables with relations that can not be truncated(because of that relations) so the only way to clear them is to use DELETE FROM <table>
statement.
Comments fix Co-authored-by: melange396 <[email protected]>
Remove unused lib Co-authored-by: melange396 <[email protected]>
Remove extra newline Co-authored-by: melange396 <[email protected]>
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 great, just a few little things i noticed...
Co-authored-by: melange396 <[email protected]>
Co-authored-by: melange396 <[email protected]>
Co-authored-by: melange396 <[email protected]>
Co-authored-by: melange396 <[email protected]>
SonarCloud Quality Gate failed.
|
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.
great job!
Summary:
Added basic integration tests for all endpoints + API Keys tests
Prerequisites:
dev