We need more tests for functions (like get_jobs, get_resultsets etc.) in treeherder-client. Lets use this bug to track it.
Hi~ I would like to work on this issue, could you give me some examples about this? Thank you:)
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
(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?
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:)
@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.
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)
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-
(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 :)
Ok, so for get_jobs method in Treeherder-client, the url goes like this. 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.  - https://github.com/mozilla/treeherder/blob/master/treeherder/client/thclient/client.py#L811  - https://treeherder.mozilla.org/api/project/mozilla-inbound/jobs/?result_set_id=15558  - https://treeherder.mozilla.org/docs/#!/project/Result_Set_list
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-
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. :)
(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 :)
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.