Closed Bug 1382345 Opened 2 years ago Closed 2 years ago

Use callback in telemetry harness to wait for ping

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement

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)
Priority: -- → P3
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: nobody → jdorlus
Status: NEW → ASSIGNED
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/26a1cd534f5d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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...
```
Flags: needinfo?(jdorlus)
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?
Flags: needinfo?(jdorlus) → needinfo?(hskupin)
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.
Flags: needinfo?(hskupin)
You need to log in before you can comment on or make changes to this bug.