-
Notifications
You must be signed in to change notification settings - Fork 1
Edge server tests not depending on remoteshell and httpclient #344
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
Conversation
borrrden
left a comment
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.
So far none of these look like end to end tests in particular. They test a single edge server for simple functional things. They could easily be handled in the dev tests that happen before the TDK is even reached so if this runtime is long they should be migrated out. In general the TDK should be for things that can only be achieved with coordination between multiple processes.
That being said, it's good that they are written. The edge server developer testing also has a pytest driven setup for testing a single edge server so it shouldn't be hard to migrate them if the decision is made.
| def _create_session( | ||
| self, scheme: str, url: str, port: int, auth: Optional[BasicAuth] | ||
| ) -> ClientSession: | ||
| return ClientSession(f"{scheme}{url}:{port}", auth=auth) |
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 method doesn't seem to add anything if it is just one line long
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.
Good point - it’s a one-liner right now, but it keeps ClientSession creation in one place and avoids duplicating the setup logic in init, so we added it
client/src/cbltest/api/edgeserver.py
Outdated
| keyspace += f".{scope}" | ||
| if collection: | ||
| keyspace += f".{collection}" | ||
| return keyspace |
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 could be simplified as:
".".join((db_name, scope, collection)).rstrip(".")
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 works, I'll change it
| self.mark_test_step( | ||
| "Waiting 20 seconds - document should expire based on expires (lower value)" | ||
| ) | ||
| time.sleep(20) |
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 would be better as 5 or so. 20 seconds is a long time for a test to sit and wait.
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 kept it at 20 seconds to be safe, but we could try 10 seconds - it should still pass reliably even with small network delays
That’s fair. The end-to-end tests depended on |
|
@borrrden I’ve made the changes you suggested. Since they’re minor, I’m planning to merge - let me know if you want to review them again |
This PR adds in all tests that have no dependence on remoteshell.py and http_client.py.
Features added:
client/src/cbltest/api/edgeserver.pyand,client/src/cbltest/api/error.py).client/src/cbltest/__init__.pyand,client/src/cbltest/configparser.py).test_blobs.py,test_crud.py, andtest_ttl_expires.py).test_blobs.py(indataset/edge-server/blobsfolder).