-
Notifications
You must be signed in to change notification settings - Fork 3
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
align test session id format with cli command #52
Conversation
c1f1bde
to
83beb53
Compare
nose_launchable/client.py
Outdated
@@ -78,12 +78,11 @@ def start(self, build_number): | |||
response_body = res.json() | |||
logger.debug("Response body: {}".format(response_body)) | |||
|
|||
self.test_session_id = response_body['id'] | |||
self.test_session_id = "builds/{}/test_sessions/{}".format( |
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.
self.test_session_id
is used to construct urls for uploading test case results and closing test sessions so I don't think changing self.test_session_id
itself is a good idea. Instead, you might want to somehow pass build_name (or build_number) to the subset method.
nose-launchable/nose_launchable/client.py
Lines 113 to 119 in 60f0192
def upload_events(self, events): | |
url = "{}/intake/organizations/{}/workspaces/{}/builds/{}/test_sessions/{}/events".format( | |
self.base_url, | |
self.org_name, | |
self.workspace_name, | |
self.build_number, | |
self.test_session_id |
nose-launchable/nose_launchable/client.py
Lines 128 to 135 in 60f0192
def finish(self): | |
url = "{}/intake/organizations/{}/workspaces/{}/builds/{}/test_sessions/{}/close".format( | |
self.base_url, | |
self.org_name, | |
self.workspace_name, | |
self.build_number, | |
self.test_session_id | |
) |
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.
Hmm, How about change to use fixed test_session_id (buils//test_sessions/ format) in uploading test case results and closing test session?
Because I think we should gather a logic that building test session id in one place.
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 could work, but then the name self.test_session_id
looks strange to me since it is not. I don't want to step into how to implement it but If I were you, I would create a new small class like TestSessionContext
and store build id and test session id to its instance. Then we can call getTestSessionId or getBuildPath to extract the necessary information.
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.
Hmm, I'm also wondering why those unite tests did not fail...
nose-launchable/tests/test_client.py
Line 217 in 83beb53
mock_requests.patch.assert_called_once_with(expected_url, headers=expected_headers) |
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 think set test_session_id directly so this test didn't fail
nose-launchable/tests/test_client.py
Lines 204 to 205 in 83beb53
client.build_number = "test_build_number" | |
client.test_session_id = "1" |
@shuheiktgw I changed that introduced TestSessionContext and use it. Could you review again? |
self.base_url, | ||
self.org_name, | ||
self.workspace_name, | ||
self.build_number, | ||
self.test_session_id | ||
self.test_session_context.get_build_path(), |
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.
Wow, I haven't noticed we could remove self.build_number but this makes sense!
1f587c1
to
3ed2c55
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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.
LGTM👍 Thank you!
launchableinc/cli outputs session
builds/{build number}/test_sessions/{test session id}
format. like below)However, nose-launchable handle test session id as
test_sessions/{test session id}
, the cli will fail to parse session https://github.com/launchableinc/cli/blob/1df90eb8ffe42ae6f7deb3f2e1fb6482ec8613f4/launchable/utils/session.py#L102-L111. So, I fixed it.Before fixing it, the cli only used the base so this issue didn't become a problem https://github.com/launchableinc/cli/blob/1df90eb8ffe42ae6f7deb3f2e1fb6482ec8613f4/launchable/commands/subset.py#L189-L191