Add tests for various get functions in treeherder-client

RESOLVED FIXED

Status

Tree Management
Treeherder: Client Libraries
P3
normal
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: vaibhav1994, Assigned: MikeLing)

Tracking

Details

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
wlach
: review+
Details | Review | Splinter Review
(Reporter)

Description

3 years ago
We need more tests for functions (like get_jobs, get_resultsets etc.) in treeherder-client. Lets use this bug to track it.
(Assignee)

Comment 1

3 years ago
Hi~ I would like to work on this issue, could you give me some examples about this? Thank you:)
(Reporter)

Comment 2

3 years ago
Mike, I was at the work-week and somehow missed this.
You can check the tests for client.py here: https://github.com/mozilla/treeherder/blob/master/tests/client/test_treeherder_client.py
(Assignee)

Comment 3

3 years ago
(In reply to Vaibhav (:vaibhav1994) from comment #2)
> Mike, I was at the work-week and somehow missed this.
> You can check the tests for client.py here:
> https://github.com/mozilla/treeherder/blob/master/tests/client/
> test_treeherder_client.py

Hm, so the things I need to do is add some tests for get_jobs and get_resultsets inside TreeherderClientTest class which is  https://github.com/mozilla/treeherder/blob/master/tests/client/test_treeherder_client.py#L410. Isn't it?
(Assignee)

Comment 4

3 years ago
Hi Vaibhav,

Could you brief something about mock_post.call_args (https://github.com/mozilla/treeherder/blob/master/tests/client/test_treeherder_client.py#L438) for me? I'm new to treeherder python client, so, I'm not about treeherder.client.client.requests.post.call_args' meaning.

Thank you:)
(Reporter)

Comment 5

3 years ago
@patch decorator makes it easy to mock classes/functions in a file. So it is mocking the post function in treeherder-client, since we do not actually want to post the test data.

Updated

2 years ago
Priority: -- → P3
(Assignee)

Comment 6

2 years ago
Created attachment 8651034 [details] [review]
PR for bug 1177531

Sorry for the late respond. I'm not sure whether I do the right way to test for it. So I decide to ask feedback for now, please give me some advice on it. Thank you very much!
Attachment #8651034 - Flags: feedback?(vaibhavmagarwal)
(Assignee)

Updated

2 years ago
Assignee: nobody → sabergeass
(Reporter)

Comment 7

2 years ago
Comment on attachment 8651034 [details] [review]
PR for bug 1177531

This does not look like the right way. You will need to make a mock get function for a request, and compare that result to a pre-known outcome.
There are various example for testing get functions lying in the codebase like: https://github.com/MikeLing/treeherder/blob/bugfix-1177531/tests/client/test_perfherder_client.py#L10

I think :wlach will give you a more detailed feedback, since he has more experience with the codebase.
Attachment #8651034 - Flags: feedback?(vaibhavmagarwal) → feedback-
(Assignee)

Comment 8

2 years ago
(In reply to Vaibhav (:vaibhav1994) from comment #7)

Cool, I know what should I do this time. Could you or :wlach tell me which api I should wrote test for them and show me the usage of them? Something like https://treeherder.mozilla.org/api/project/mozilla-inbound/performance-data/get_performance_series_summary/?interval=86400, so I can understand what's the json data looks like and how to mock them. 

Thank you Vaibhav, great appreciated :)
(Reporter)

Comment 9

2 years ago
Ok, so for get_jobs method in Treeherder-client[1], the url goes like this[2]. Similarly for get_resultset method, you can construct the url from the method parameters. A good documentation for these urls is given in treeherder docs[3].

[1] - https://github.com/mozilla/treeherder/blob/master/treeherder/client/thclient/client.py#L811
[2] - https://treeherder.mozilla.org/api/project/mozilla-inbound/jobs/?result_set_id=15558
[3] - https://treeherder.mozilla.org/docs/#!/project/Result_Set_list
(Assignee)

Updated

2 years ago
Attachment #8651034 - Flags: review?(vaibhavmagarwal)
(Reporter)

Comment 10

2 years ago
Comment on attachment 8651034 [details] [review]
PR for bug 1177531

William might be a better reviewer for this :)

Left a comment in the PR.
Attachment #8651034 - Flags: review?(vaibhavmagarwal) → review?(wlachance)
Comment on attachment 8651034 [details] [review]
PR for bug 1177531

I like where this is going but would like to see some changes before it goes in.
Attachment #8651034 - Flags: review?(wlachance) → review-
(Assignee)

Updated

2 years ago
Attachment #8651034 - Flags: review- → review?(wlachance)

Comment 12

2 years ago
Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/6af23d435679cec1a672a01635dd2f888b76e312
Bug 1177531 - Add tests for various get functions in treeherder-client

https://github.com/mozilla/treeherder/commit/1b4c8aeb5a59faf58d0a3f5ca5366717325ae1d2
Merge pull request #894 from MikeLing/bugfix-1177531

Bug 1177531 - Add tests for various get functions in treeherder-client
Comment on attachment 8651034 [details] [review]
PR for bug 1177531

This is good. Merged. There are still quite a few other methods to test, so leaving this bug open for now. :)
Attachment #8651034 - Flags: review?(wlachance)
Attachment #8651034 - Flags: review+
Attachment #8651034 - Flags: feedback-
(Assignee)

Comment 14

2 years ago
(In reply to William Lachance (:wlach) from comment #13)
> Comment on attachment 8651034 [details] [review]
> PR for bug 1177531
> 
> This is good. Merged. There are still quite a few other methods to test, so
> leaving this bug open for now. :)

ok, please tell me if you found any function need test, and i would happy to work on that :)

Comment 15

11 months ago
Some tests did already land, we can always open another bug for more tests.
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.