test: Add e2e test for streaming updates python client #8901
Conversation
✅ Deploy Preview for determined-ui canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8901 +/- ##
==========================================
- Coverage 47.21% 47.21% -0.01%
==========================================
Files 1162 1162
Lines 175913 175913
Branches 2237 2235 -2
==========================================
- Hits 83062 83059 -3
- Misses 92693 92696 +3
Partials 158 158
Flags with carried forward coverage won't be shown. Click here to find out more. |
e2e_tests/pytest.ini
Outdated
| @@ -13,6 +13,7 @@ markers = | |||
| e2e_cpu_postgres: end to end CPU tests for testing basic database functionality | |||
| e2e_cpu_cross_version: end to end CPU tests for testing basic cluster functionality with differing master/agent versions | |||
| e2e_cpu_agent_connection_loss: end to end CPU tests for testing agent functionality on connection loss | |||
| e2e_cpu_streaming: end to end streaming updates tests | |||
There was a problem hiding this comment.
why add a new kind of test? Why not include this in the normal cpu test suite?
There was a problem hiding this comment.
I wonder what is the rule for splitting tests? I saw rbac, elastic being tested separately
There was a problem hiding this comment.
there really isn't a rule.
The rbac tests are EE only, so that makes sense.
The elastic tests require a special cluster, so that also makes sense.
But these tests aren't very special, they shouldn't get their own suite.
|
|
||
| from determined.common.api import bindings | ||
| from determined.common.streams import wire | ||
| from determined.common.streams._client import LomondStreamWebSocket, ProjectSpec, Stream, Sync |
There was a problem hiding this comment.
don't import symbols directly; only import modules.
In this case, from determined.common import streams and use streams.LomondStreamWebSocket, streams.ProjectSpec, etc.
| newProjectName = "streaming_project_1" | ||
|
|
||
| resp_w = bindings.post_PostWorkspace( | ||
| sess, body=bindings.v1PostWorkspaceRequest(name=f"workspace_aug_{uuid.uuid4().hex[:8]}") |
There was a problem hiding this comment.
how about codecs.encode(random.randbytes(4), 'hex') instead?
Using uuid.uuid4() to get random bytes is odd.
| assert event == wire.ProjectsDeleted(str(p.id)) | ||
| break | ||
| else: | ||
| raise Exception("Unexpected message from stream.") |
There was a problem hiding this comment.
don't raise a bare Exception. Use a ValueError if you have to do something whose meaning doesn't convey much.
Also, please include the message in the exception, so that if anybody were to hit the error in CI they'd see the message that caused the problem in its entirety.
gt2345
force-pushed
the
gt/282-py-client-test
branch
from
5c885b2
to
e39b2f6
Compare
gt2345
force-pushed
the
gt/282-py-client-test
branch
from
cd4ed53
to
21f8139
Compare
|
|
||
| from determined.common import streams | ||
| from determined.common.api import bindings | ||
| from determined.common.streams import _client |
There was a problem hiding this comment.
You are importing the modules, yes, but in the case of the streams._client module, there is nothing that calling code should be importing directly.
The full code layout rfc is here if you want to read it.
TL;DR is: you can generally assume that any of our python files that start with an underscore are not meant to be imported directly, and the relevant symbols should be available in their parent namespace.
So in this case, all the symbols you get from _client.* should actually be accessed as stream.*.
You can think of this as a way to separate the organization of source code (which code goes in which _-prefixed files) from the import path for accessing exported symbols.
Description
Add e2e test for streaming updates python client
Test Plan
e2e tests pass
Commentary (optional)
Checklist
docs/release-notes/.See Release Note for details.
Ticket
MD-282