pass a UserClientSet into SDKMethodRunner #296

MrCreosote · 2021-01-28T22:32:50Z

Description of PR purpose/changes

UCS contains the clients that need to be instantiated on a per user basis.
UCS is passed in to SDKMR to 1) simplify the SDKMR constructor, and 2) allow for eventual unit testing of SDKMR, since the UCS and workspace client can be mocked via create_autospec.

Also add instructions for running pre-commit hooks.

Jira Ticket / Github Issue

[n/a] Added the Jira Ticket to the title of the PR e.g. (DATAUP-69 Adds a PR template)

Testing Instructions

Details for how to test the PR:

Tests pass in travis and locally
Changes available by spinning up a local test suite

Dev Checklist:

My code follows the guidelines at https://sites.google.com/truss.works/kbasetruss/development
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
[?] My changes generate no new warnings - too many warnings to feasibly check
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
[n/a] Any dependent changes have been merged and published in downstream modules
I have run Black and Flake8 on changed Python Code manually or with git precommit (and the travis build passes)

Updating Version and Release Notes (if applicable)

[n/a] Version has been bumped for each release
[n/a] Release notes have been updated for each release (and during the merge of feature branches)

UCS contains the clients that need to be instantiated on a per user basis. UCS is passed in to SDKMR to allow for eventual unit testing of SDKMR, since the UCS and workspace client can be mocked via create_autospec.

MrCreosote · 2021-01-28T22:34:28Z

Huh. The failing tests were marked as failing immediately, and failing after [2, 8]m, even thought they couldn't have started yet

MrCreosote · 2021-01-28T22:35:17Z

lib/execution_engine2/constants.py

@@ -0,0 +1,5 @@
+'''


I had to pull the constants into a separate file to avoid an import cycle

MrCreosote · 2021-01-28T22:36:07Z

lib/execution_engine2/utils/clients.py

+        return self._workspace_auth
+
+
+class ClientSet:


Currently unused, but is the next step in the unit-testability work

lgtm-com · 2021-01-28T22:55:16Z

This pull request introduces 3 alerts when merging 5c01612 into 441293c - view on LGTM.com

new alerts:

3 for Unused import

Also added pre-commit instructions

Surprised flake8 didn't highlight these

... a 1 line triple quoted comment unless the quotes are one the same line.

lib/execution_engine2/constants.py

MrCreosote · 2021-01-29T19:09:11Z

I see a fix I can make for the workspace-url check in the client set, give me a sec

MrCreosote · 2021-01-29T19:31:07Z

@bio-boris ok, all set, ready for re-review

bio-boris · 2021-02-01T08:15:48Z

lib/execution_engine2/utils/clients.py

+        self._workspace = Workspace(ws_url, token=token)
+        self._workspace_auth = WorkspaceAuth(user_id, self._workspace)
+
+    # create_autospec can't mock instance variables, so we add some boilerplate


Seems we just need to pass spec_set=True to magicmocks to achieve the same functionality as createautospec and then there is no need to create setters and getters for instance variables just for testing purposes?

Have you verified that with a @patch decorator with spec_set=True that it detects instance variables? My understanding is that the spec_set mocks can't see instance variables regardless of how the mock is created. I'll update the comment to that effect.

Ah, No, instance variables are not detected at all, so nevermind.

Removed getters, no longer needed

bio-boris · 2021-02-01T16:23:28Z

lib/execution_engine2/execution_engine2Impl.py

@@ -41,6 +42,10 @@ class execution_engine2:

    ADMIN_ROLES_CACHE_SIZE = 500
    ADMIN_ROLES_CACHE_EXPIRE_TIME = 300  # seconds
+
+    def get_user_clients(self, ctx) -> UserClientSet:


Is there an advantage of having this here instead of inside the SDKMethodRunner?

yes, instantiating the client set outside of SDKMR is what allows for passing mock objects into SDKMR rather than monkey patching (although you can still do that if you want) and enables constructor-based dependency injection. The major change for this PR is exactly that - making the code that creates SDKMR responsible for the creation of SDKMR's dependencies rather than SDKMR.

I should have said inside the SDKMethodRunner file, defined outside of the SDKMethodRunner class, or moving this function to its own file. We should probably move out everything from the BEGIN_CLASS_HEADER section to its own file, as there are often issues with editing code in the impl file, and we have the common KBase pattern of doing as little as possible in the IMPL file itself.

Oh I see. I'll make an APIHelper class that holds a reference to the config so we don't have to pass it around all over place and put the method there.

bio-boris · 2021-02-01T16:30:10Z

lib/execution_engine2/utils/clients.py

+        self.auth_admin = AdminAuthUtil(auth_url, [ADMIN_READ_ROLE, ADMIN_WRITE_ROLE])
+
+        # KafkaClient has a nice error message when the arg is None
+        self.kafka_client = KafkaClient(cfg.get("kafka-host"))


Maybe we should follow the fail fast with Keyerror way to handling dictionaries by doing the following

cfg['key'] == required value cfg.get('key') == optionally None cfg.get('key','default') == explicitly set default

If we wanted to do something like this the way it'd do it is something like:

if not cfg.get('key') or not cfg.get('key').strip(): raise ValueError('configuration is missing key "key"')

Just getting a KeyError("key") is not very helpful to someone trying to figure out why the service won't start, especially if they're not familiar with it.

I actually did use your first approach in cases where the client I was instantiating ignored missing arguments, although there could still be issues if the argument was whitespace or a bad url, etc. Where the clients had readable error messages (like Kafka and Slack) I went with those.

In most of my repos all the config args are checked for correctness, and urls used to make requests to the service and check the results, before the service starts. The checking is mostly delegated to the clients / objects that use them since they understand the semantics.

Add dict['key'] vs dict.get('') to the style guide at #303

bio-boris · 2021-02-01T20:27:42Z

lib/execution_engine2/utils/clients.py

+    # Could make these properties but then they return MagicMocks which don't update with API
+    # changes
+
+    def user_id(self):


This seems strange to do it this way. Are other kbase codebases doing it this way?

I don't think so. You only need to do this when you have a mock where you need to mock the instance variables, which doesn't happen all that often in my experience.

A better way to do this might be to make a constructor that takes a workspace client, user id, and token, and then you can just mock the workspace client and you don't need to mock the client set. I think I'll do that, then this becomes a non-issue.

Did that, this is now moot, no more getters

bio-boris · 2021-02-01T20:30:31Z

test/tests_for_auth/ee2_admin_mode_test.py

        # Regular User
        lowly_user = "Access Denied: You are not an administrator"
-        runner = self.getRunner()
+        ucs, wsa = self.get_mocks()


It would help with readability to write out
user_client_set and ws_auth instead of these 3 letter variables names

test/tests_for_sdkmr/ee2_SDKMethodRunner_EE2Logs_test.py

Effectively makes it a value class which doesn't need to be mocked. Just pass mocks into the UCS instead.

Keeping as much code as possible out of the Impl file is good practice

I'm looking at my console right now, and I see a black run with no issues, and then the current black run with this problem. I don't understand why black didn't catch this the first time.

It's needed in a couple of places, most notably EE2RunJob

bio-boris

Created new issues, questions answered, LGTM

MrCreosote · 2021-02-04T00:31:36Z

Travis CI should be deleted per Boris

* Remove cruft.py (#279) I suspect it may contain some cruft. * Correct spec, improve documentation (#284) * Remove cruft.py I suspect it may contain some cruft. * Correct spec, improve documentation Corrected / improved spec and spec documentation for date range methods * Fix spec typo. Also clarify what needs to be done to run tests after making code changes * make WorkspaceAuth and authstrategy unit-testable (#288) * Remove cruft.py I suspect it may contain some cruft. * Correct spec, improve documentation Corrected / improved spec and spec documentation for date range methods * Fix spec typo. Also clarify what needs to be done to run tests after making code changes * make WorkspaceAuth and authstrategy unit-testable Inject dependencies so they can be mocked. * Fix flake8 * python formatting rules are stupid * more flake8 fixes really, flake8? really? * Make token and user id required for sdkmr (#294) Always provided, so no reason to be optional, and makes the code easier to understand. * pass a UserClientSet into SDKMethodRunner (#296) * pass a UserClientSet into SDKMethodRunner UCS contains the clients that need to be instantiated on a per user basis. UCS is passed in to SDKMR to allow for eventual unit testing of SDKMR, since the UCS and workspace client can be mocked via create_autospec * remove extraneous period Co-authored-by: MrCreosote <[email protected]> Co-authored-by: Gavin <[email protected]>

pass a UserClientSet into SDKMethodRunner

79eed76

UCS contains the clients that need to be instantiated on a per user basis. UCS is passed in to SDKMR to allow for eventual unit testing of SDKMR, since the UCS and workspace client can be mocked via create_autospec.

MrCreosote requested a review from bio-boris January 28, 2021 22:32

MrCreosote commented Jan 28, 2021

View reviewed changes

typos

5c01612

MrCreosote added 3 commits January 28, 2021 15:08

I thought I ran pre-commmit on this but I guess not

b074b38

Also added pre-commit instructions

Remove unused imports

e7f8c96

Surprised flake8 didn't highlight these

Codacy won't let you have...

ebf5671

... a 1 line triple quoted comment unless the quotes are one the same line.

bio-boris reviewed Jan 29, 2021

View reviewed changes

lib/execution_engine2/constants.py Outdated Show resolved Hide resolved

Combine constants files

f456931

MrCreosote added 2 commits January 29, 2021 11:24

test that workspace url is provided in user client set

25b8dfc

remove done TODO

f138b68

bio-boris reviewed Feb 1, 2021

View reviewed changes

test/tests_for_sdkmr/ee2_SDKMethodRunner_EE2Logs_test.py Outdated Show resolved Hide resolved

MrCreosote added 3 commits February 3, 2021 13:17

Remove need to mock UserClientSet

4cc92c3

Effectively makes it a value class which doesn't need to be mocked. Just pass mocks into the UCS instead.

Move helper function out of Impl file

6781f17

Keeping as much code as possible out of the Impl file is good practice

Run black (again?)

2321e2b

I'm looking at my console right now, and I see a black run with no issues, and then the current black run with this problem. I don't understand why black didn't catch this the first time.

MrCreosote mentioned this pull request Feb 3, 2021

Determine a style guide #303

Open

Remove TODO

0e610a3

It's needed in a couple of places, most notably EE2RunJob

bio-boris approved these changes Feb 4, 2021

View reviewed changes

MrCreosote merged commit de6cbab into dev-require_resources Feb 4, 2021

MrCreosote deleted the dev-user-client-set branch February 8, 2021 03:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pass a UserClientSet into SDKMethodRunner #296

pass a UserClientSet into SDKMethodRunner #296

MrCreosote commented Jan 28, 2021 •

edited

Loading

MrCreosote commented Jan 28, 2021

MrCreosote Jan 28, 2021

MrCreosote Jan 28, 2021

lgtm-com bot commented Jan 28, 2021

MrCreosote commented Jan 29, 2021

MrCreosote commented Jan 29, 2021

bio-boris Feb 1, 2021

MrCreosote Feb 1, 2021

bio-boris Feb 1, 2021

MrCreosote Feb 3, 2021

bio-boris Feb 1, 2021

MrCreosote Feb 3, 2021

bio-boris Feb 3, 2021

MrCreosote Feb 3, 2021

MrCreosote Feb 3, 2021

bio-boris Feb 1, 2021

MrCreosote Feb 3, 2021 •

edited

Loading

bio-boris Feb 4, 2021 •

edited

Loading

bio-boris Feb 1, 2021

MrCreosote Feb 3, 2021

MrCreosote Feb 3, 2021

bio-boris Feb 1, 2021

MrCreosote Feb 3, 2021

bio-boris left a comment

MrCreosote commented Feb 4, 2021

pass a UserClientSet into SDKMethodRunner #296

pass a UserClientSet into SDKMethodRunner #296

Conversation

MrCreosote commented Jan 28, 2021 • edited Loading

Description of PR purpose/changes

Jira Ticket / Github Issue

Testing Instructions

Dev Checklist:

Updating Version and Release Notes (if applicable)

MrCreosote commented Jan 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Jan 28, 2021

MrCreosote commented Jan 29, 2021

MrCreosote commented Jan 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrCreosote Feb 3, 2021 • edited Loading

Choose a reason for hiding this comment

bio-boris Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bio-boris left a comment

Choose a reason for hiding this comment

MrCreosote commented Feb 4, 2021

MrCreosote commented Jan 28, 2021 •

edited

Loading

MrCreosote Feb 3, 2021 •

edited

Loading

bio-boris Feb 4, 2021 •

edited

Loading