-
Notifications
You must be signed in to change notification settings - Fork 45
Add support for Globus Tunnels #1351
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
|
@buzztroll A changelog fragment is needed. This can be generated using |
620ba21 to
0e2ae94
Compare
sirosen
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.
The main thing we're missing here is tests. We need dummy responses for each of these APIs in src/globus_sdk/testing/data/ and unit tests which run the various methods.
We aren't at perfect coverage today, but we are aiming for 100% on any new work in the SDK.
My overall request is "an initial crack at testing, across the various new methods and new object", not "perfect and exhaustive testing".
I went back through the PR history to find useful reference PRs, and I've got two which should provide decent guidance.
Testing a New API
The most recent new-API was "Update Index" for Search, and looks like a pretty good example: https://github.com/globus/globus-sdk-python/pull/1310/files
Importantly! That PR demonstrates that you don't need to go through a lot of effort to build up some kind of fancy modeling of the responses. Some fixed mocks which have the right shape are totally fine. There are more sophisticated tests with more dynamic responses and whatnot, but we're literally just looking in the base case to make the testsuite run the lines added in src/globus_sdk/services/transfer/client.py.
Testing a New Payload Type
The most recent new-Payload-type was "FlowTimer" for Timers. It has some more complexity in modeling responses, but I really want to point at the test, and the way it "pulls back" the payload after sending it, to examine what was sent:
https://github.com/globus/globus-sdk-python/pull/1311/files
Using
req = get_last_request()
sent = json.loads(req.body)lets us call the relevant method and then assert on what we actually would send to the API.
That might also be a nice way to test the handling of submission_id.
5e78e78 to
a77bb85
Compare
derek-globus
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.
Sorry for the slow response on this. Overall I am happy with the direction. A few smaller to think about yet before merging though.
sirosen
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.
I just noticed that I had some pending comments which overlap with Derek's. Posting so they are visible.
5e7b338 to
7ebe8c2
Compare
8f3f982 to
cd76dbd
Compare
sirosen
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.
Very minor final tweaks needed for us to merge, but I'm approving. I'll merge once we have those last things buttoned up.
f85a8e6 to
d3f00b8
Compare
This patch adds methods to the TransferClient that will allow for interaction with the Globus Streams functionality.
d879c83 to
7154745
Compare
|
🎄 🎅 🤶 |
It includes the API calls needed by programs like the globus-cli.