UIP-49 First steps on narrativemanager updating. #99
Conversation
| self._get_set_api_client(ctx["token"]), | ||
| self._get_data_palette_client(ctx["token"]), |
There was a problem hiding this comment.
These aren't actually used anywhere, so removed from the narrative manager module.
| @@ -56,7 +58,7 @@ def auth_client(config: dict[str, str | int]) -> Generator[KBaseAuth, None, None | |||
| yield KBaseAuth(config["auth-service-url"]) | |||
|
|
|||
| @pytest.fixture(scope="module") | |||
| def context(auth_token: str, auth_client: KBaseAuth) -> Generator[dict[str, any], None, None]: | |||
| def context(auth_token: str, auth_client: KBaseAuth) -> Generator[dict[str, Any], None, None]: | |||
There was a problem hiding this comment.
any is a builtin function. typing.Any is the type. Oops. (more in the next PR).
| @@ -0,0 +1,142 @@ | |||
| """ | |||
There was a problem hiding this comment.
Gave this the pytest treatment, but mostly left it unchanged.
There was a problem hiding this comment.
I tried really hard not to change any actual code here. Most of the changes are either linting, typing, or renaming variables from camelCase to snake_case. I'll point out any real changes.
| self.set_api_client = set_api_client # DynamicServiceCache type | ||
| self.data_palette_client = data_palette_client # DynamicServiceCache type |
There was a problem hiding this comment.
These are set, then never used by the module. So they've been removed.
| data = None | ||
| while tries < 60 and data is None: | ||
| while tries < max_tries and data is None: |
There was a problem hiding this comment.
Still a "magic number" but passes linting.
A future PR will clean this up further, and likely export it to a different module.
| @@ -21,24 +25,32 @@ class NarrativeManager: | |||
| KB_CODE_CELL = "kb_code" | |||
| KB_STATE = "widget_state" | |||
|
|
|||
| def __init__(self, config, user_id, set_api_client, data_palette_client, workspace_client, search_service_client): | |||
| def __init__( | |||
| self, | |||
There was a problem hiding this comment.
The selfs can all have the class "NarrativeManager" (with quotes)
| time_ms = int(round(time.time() * 1000)) | ||
| newWsName = self.user_id + ":narrative_" + str(time_ms) | ||
| new_ws_name = self.user_id + ":narrative_" + str(time_ms) |
There was a problem hiding this comment.
I love that it suddenly starts using camel case halfway through the file!
There was a problem hiding this comment.
The person who originally wrote this (and other chunks of NarrativeService) was primarily a Java programmer. Also, this particular file was basically directly translated from a JavaScript module. I guess naming conventions came along for the ride.
| with pytest.raises(ValueError, match="required format is <workspace_id>/<object_id>/<version>"): | ||
| nm.get_narrative_doc("blah/blah/blah/blah") |
There was a problem hiding this comment.
what happens if the UPA is in the format ws_name/obj_name?
For these tests, is it worth creating a test workspace with the various needed objects and then recording the responses with vcrpy?
There was a problem hiding this comment.
is it worth creating a test workspace with the various needed objects?
I think that's worth doing for integration tests, once those get reworked. I think the issue here is that we're testing modifications to existing data. For the unit tests, I think I want to just test the NarrativeManager code in question - does it fire off the right backend call ok and format results properly. This currently does kind of a weird hodge-podge that, IMO, isn't necessary.
Integration tests should go all in, make a new Workspace, put data in it, modify it, see if the Workspace service responds right, changes the data as expected, and so on.
There was a problem hiding this comment.
Mostly, I'd like to just pytest + ruff in this PR, keep all the warts the same for consistency, then go back in the next PR and rework the process a bit.
test/unit/test_narrativemanager.py
Outdated
| nm = NarrativeManager(config, mock_user, mock_workspace_client, mock.MagicMock()) | ||
|
|
||
| # simulate reverting fake narrative to version #2 | ||
| # (make_object_history=True automatically makes 5 versions) |
There was a problem hiding this comment.
could you put this comment up next to where you create the narrative?
test/unit/test_narrativemanager.py
Outdated
| make_object_history=True | ||
| ) | ||
|
|
||
| (ws_id, obj, _) = narrative_ref.split("/") |
There was a problem hiding this comment.
worth checking the version of the narrative ref here to ensure there are five versions?
|
Responded to comments, and made a few requested changes. |
First steps here for the NarrativeManager module fixup. These include: