-
Notifications
You must be signed in to change notification settings - Fork 8
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 tests #520
Integration tests #520
Conversation
25c941a
to
b053eeb
Compare
b053eeb
to
17f87e5
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.
nice job with getting integration tests started.
|
||
def _create_project(self): | ||
project_id = generate_code() | ||
project_auth_token = generate_code() |
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.
why don't you use the model for the project? You replicate internal project creation logic above.
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'm embarrassed to say I forgot about those wonderful helpful models. I'll use the Project
one here.
project = get_project(conn, project_id) | ||
assert 'time_added' in project | ||
assert not project['marked_for_deletion'] | ||
assert not project['uses_blocking'] |
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.
you are not really testing if the project you got back is the project you put in.
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.
Fair
backend/entityservice/integrationtests/dbtests/test_insertions.py
Outdated
Show resolved
Hide resolved
|
||
end_time = time.perf_counter() | ||
elapsed_time = end_time - start_time | ||
assert elapsed_time < 1 |
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.
why 1? why not 10, or 0.1? What if the database is under heavy load and this fails?
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'll add a comment and increase it. On my desktop it takes <0.5s to insert 10k 128B elements + blocks into a local db. We both probably agree this may not be fast enough, but I wanted to start failing a test if it got any worse.
assert elapsed_time < 1 | ||
|
||
stored_encodings = list(get_encodingblock_ids(conn, dp_id, '1')) | ||
assert len(stored_encodings) == num_entities |
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.
why don't you test for equality of the encodings?
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 actually changed the db a bit and added new selection functions that would be helpful in the branch feature-use-encodings-from-db so this will change. I'll update it to be clearer for now though - it is actually just pulling the encoding ids - not the encodings themselves back from the db.
from entityservice.cache.connection import connect_to_redis | ||
from entityservice.cache.helpers import _get_run_hash_key | ||
|
||
logger = structlog.get_logger() | ||
|
||
|
||
def save_current_progress(comparisons, run_id): | ||
def save_current_progress(comparisons, run_id, config=None): |
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 find it confusing to be able to pass config
in here, as almost all of it gets ignored anyway. It'd be clearer if it's just a cache expiry value. But then, why would you want to set a different expiry value in the first place?
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 you'd only want to do that for testing. My logic is that it would be nice to slowly move away from using the global Config
as we add tests.
Thanks for the review @wilko77 I think I've addressed your points well enough to merge. But happy to keep the conversation going as the integration tests take shape |
We currently have end to end tests that use just the REST api, and a few (not many) unit tests that test isolated Python functions.
This PR starts to add integration tests which have direct access to the support services - e.g. direct access to redis or postgres to check and/or manipulate the state. This is useful as it allows us to have smaller units of testable code relying on support services.