Closed
Bug 1382345
Opened 7 years ago
Closed 7 years ago
Use callback in telemetry harness to wait for ping
Categories
(Toolkit :: Telemetry, enhancement, P3)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: Silne30, Assigned: Silne30)
Details
Attachments
(1 file)
callback argument here so that we have something like: def send_ping(self, callback): cur_pings = len(self.ping_list) callback() Wait().until(len(self.ping_list > cur_pings)
Updated•7 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8890543 [details] Bug 1382345 - Use callback in test harness https://reviewboard.mozilla.org/r/161622/#review166982 ::: commit-message-9eddb:1 (Diff revision 1) > +Bug 1382345 - Use callback in test harness Why? This pattern seems odd where you call the callback _before_ the body of the function and pass it no information. It seems as though the only point is to have it call a function just after taking the length of the ping list. Is this "callback" more of some kind of 'action function' or something? Something you expect to change the length of the ping_list? If so, it should be named something different than callback, which has deep connotations.
Attachment #8890543 -
Flags: review?(chutten) → review-
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jdorlus
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8890543 [details] Bug 1382345 - Use callback in test harness https://reviewboard.mozilla.org/r/161622/#review166982 > Why? This pattern seems odd where you call the callback _before_ the body of the function and pass it no information. > > It seems as though the only point is to have it call a function just after taking the length of the ping list. > > Is this "callback" more of some kind of 'action function' or something? Something you expect to change the length of the ping_list? > > If so, it should be named something different than callback, which has deep connotations. I renamed the function, action_func since it is an action that is just being called. I also opened https://bugzilla.mozilla.org/show_bug.cgi?id=1384735 to create some docs in code so that arguments are not so ambiguous.
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8890543 [details] Bug 1382345 - Use callback in test harness https://reviewboard.mozilla.org/r/161622/#review167336 The commit message still refers to callbacks, but that's probably okay given the written record in the bug.
Attachment #8890543 -
Flags: review?(chutten) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/26a1cd534f5d Use callback in test harness r=chutten
Keywords: checkin-needed
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/26a1cd534f5d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8890543 [details] Bug 1382345 - Use callback in test harness https://reviewboard.mozilla.org/r/161622/#review168728 ::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:57 (Diff revision 2) > time.sleep(5) > > - def wait_for_ping(self, ping_filter_func): > - if len(self.ping_list) == 0: > + def wait_for_ping(self, action_func, ping_filter_func): > + current_num_pings = len(self.ping_list) > + if callable(action_func): > + action_func() What happens if it is not callable? From what I see we run into a timeout without giving a proper error message. The condition here should better have been something like: ``` if not callable(): raise Exception("Must be a callable... ```
Updated•7 years ago
|
Flags: needinfo?(jdorlus)
Assignee | ||
Comment 9•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8890543 [details] Bug 1382345 - Use callback in test harness https://reviewboard.mozilla.org/r/161622/#review168728 > What happens if it is not callable? From what I see we run into a timeout without giving a proper error message. The condition here should better have been something like: > > ``` > if not callable(): > raise Exception("Must be a callable... > ``` OK. That makes sense. Certain test cases require no action func so I was passing a value of None to the function. That, of course, is not callable and would throw the exception. Should I pass an empty lambda function in those situations?
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jdorlus) → needinfo?(hskupin)
Comment 10•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8890543 [details] Bug 1382345 - Use callback in test harness https://reviewboard.mozilla.org/r/161622/#review168728 > OK. That makes sense. Certain test cases require no action func so I was passing a value of None to the function. That, of course, is not callable and would throw the exception. Should I pass an empty lambda function in those situations? If you want to make yourself independent from having to reset the ping value to 0 all the time, you need a callback. I cannot see test cases which wouldn't need that. You would always introduce race conditions in those cases.
Updated•7 years ago
|
Flags: needinfo?(hskupin)
You need to log in
before you can comment on or make changes to this bug.
Description
•