1380748 - The ping list has to be reset in setUp

Status: NEW

(Toolkit :: Telemetry, enhancement, P4)

Description

[John Dorlus [:Silne30]]

The test runs and passes fine. But if you repeat using the --repeat flag in marionette, each run will leave a ping cached and self.ping_list will continue to grow by factor of one per repeat. This needs to be investigated.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This is not only between test runs but each individual test. Because the list never gets reset in setUp.

Comment 2

[John Dorlus [:Silne30]]

Attachment: Bug 1380748 - Cleared ping list in setup

To prevent ping caching, cleared ping list in setup to reset its state.

Review commit: https://reviewboard.mozilla.org/r/161700/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161700/

Comment 3

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8890562 [details]
Bug 1380748 - Cleared ping list in setup

https://reviewboard.mozilla.org/r/161700/#review167922

Comment 4

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s b1eb14ebe08f -d 01309b4bee77: rebasing 410408:b1eb14ebe08f "Bug 1380748 - Cleared ping list in setup r=maja_zf" (tip)
merging toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py
warning: conflicts while merging toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8890562 [details]
Bug 1380748 - Cleared ping list in setup

https://reviewboard.mozilla.org/r/161700/#review168724

::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:55
(Diff revision 1)
>  
>          # Wait 5 seconds to ensure that telemetry has reinitialized
>          time.sleep(5)
>  
> +        # Clear ping list before running test.
> +        del self.ping_list[:]

Why are you deleting the list but not resetting it to just an empty array? After that line `self.ping_list` will no longer be available and any code which references it will fail.

Comment 6

[John Dorlus [:Silne30]]

Comment on attachment 8890562 [details]
Bug 1380748 - Cleared ping list in setup

https://reviewboard.mozilla.org/r/161700/#review168724

> Why are you deleting the list but not resetting it to just an empty array? After that line `self.ping_list` will no longer be available and any code which references it will fail.

I am not sure I agree with that assessment. The items in the list are being deleted, not the actual array itself. Hence the [:]. Let me know if you still want me to change this.  https://stackoverflow.com/questions/14465279/delete-all-objects-in-a-list

Comment 7

[John Dorlus [:Silne30]]

Comment on attachment 8890562 [details]
Bug 1380748 - Cleared ping list in setup

https://reviewboard.mozilla.org/r/161700/#review168724

> I am not sure I agree with that assessment. The items in the list are being deleted, not the actual array itself. Hence the [:]. Let me know if you still want me to change this.  https://stackoverflow.com/questions/14465279/delete-all-objects-in-a-list

The test runs and repeats fine. Repeated x20. All passed.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8890562 [details]
Bug 1380748 - Cleared ping list in setup

https://reviewboard.mozilla.org/r/161700/#review168724

> The test runs and repeats fine. Repeated x20. All passed.

Oh! I didn't know that. Using this way compared to `ping_list = []` will make sure that all other references also get the entries updated. I learned something new. So yes, it is fine. Nothing to change here.