788531 - Revise default delay for endurance test to make scenarios more realistic

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Automation, defect, P2)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

After investigating bug 784793 it has been suggested that we should revise the default delay for endurance tests. The options discussed so far:

1. Allow individual tests to set a minimum delay
2. Increase the default delay in the automation script
3. Increase the delay used by Mozmill CI
4. Set base delay, so it can be increased via the command line but not decrease

The first option means tests would likely be inconsistent. This isn't necessarily an issue, but it might make reviewing the reports misleading.

Option two allows the delay to be overridden with a lower value for exploratory testing, and keeps the delay consistent.

With option three we would have to explain to anyone using the default value why there may be misleading results due to short delays.

The fourth option sounds similar to option two, but doesn't allow for exploratory testing of shorter delays.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I think option two would be most suitable, but I'm keen to hear the thoughts of others.

Vlad: Would you be interested in finding a suitable default? You can base investigation on testTabbedBrowsing_OpenNewWindow. We know that 1s is not enough, but 10s may be more than we need.

We also need to consider what this will do to the baselines and duration for endurance tests.

Comment 2

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

I agree with Dave in making use of option 2. We could put this as an action item for next week.

Comment 3

[Maniac Vlad Florin (:vladmaniac)]

I am going to assign this to myself based on comment 1

Comment 4

[Maniac Vlad Florin (:vladmaniac)]

After investigating this yesterday, we can observe a considerable memory decrease when having a 5s default delay

http://mozmill-crowd.blargon7.com/#/endurance/report/d11b1de413a0179d904e7372300ccc18

But my opinion is that we modify the delay only for this test, because having 5s delay over other tests do not make them realistic at all. Thats just my opinion.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Vlad: What other values did you try? Could you provide a few more reports so we can compare them?

Why would having a 5 second delay for other tests not make them more realistic? We're not suggesting that a user would do the same thing repeatedly every 5 seconds, we're just saying that they would probably never perform any action 0.1 seconds after their previous action.

As mentioned in comment 0:
> 1. Allow individual tests to set a minimum delay
> The first option means tests would likely be inconsistent. This isn't necessarily an issue, but it might make reviewing the reports misleading.

Comment 6

[Maniac Vlad Florin (:vladmaniac)]

OK then I will just follow up with reports then. 
I've stopped at 5 seconds, but would be nice to know a threshold of memory that we want to see for opening new window. That way I can have a clear objective

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Well with the 5s report it appears to me that roughly half of the explicit memory allocated when opening the windows is cleared after closing them. Ideally all memory allocated would be released. You can also see an increasing trend in the resident memory, which would ideally not be present unless there was a memory leak.

If the above is not achievable within a reasonable delay, then we can compromise. At the moment, 5s is looking like a huge improvement over the current default delay.

Comment 8

[Maniac Vlad Florin (:vladmaniac)]

At 6s already the window test disconnects http://mozmill-crowd.blargon7.com/#/endurance/report/d11b1de413a0179d904e737230149b30

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I suspect that opening a new window is not resetting the Mozmill timeout, and therefore 60+delay (66) seconds is being reached and Mozmill disconnects. Could you raise a separate bug to investigate this, but for now you can increase the value locally here http://hg.mozilla.org/qa/mozmill-automation/file/416592141962/libs/testrun.py#l625

Comment 10

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

We should never run into this global timeout which would only kick in after 66s in this case. We always have other stuff ongoing and never wait that long.

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

That's why we need to open a bug to investigate it - because we are hitting the global timeout, and I have seen this before. When you say "We always have stuff ongoing" we really need to define "stuff". In case of this test, there are no mouse clicks or keyboard interactions, so perhaps the opening/closing of windows does not reset the timeout...

Comment 12

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

We are using waitFor() which should fire an event through JSBridge. Would you mind to check in Mozmill 1.5 if we really initiate such a pass event? If not then we have to fix it in Mozmill itself. That's the only thing which comes into my mind right now why we are failing here.

Comment 13

[Andrew McCreight [:mccr8]]

(In reply to Dave Hunt (:davehunt) from comment #7)
> Well with the 5s report it appears to me that roughly half of the explicit
> memory allocated when opening the windows is cleared after closing them.
> Ideally all memory allocated would be released. You can also see an
> increasing trend in the resident memory, which would ideally not be present
> unless there was a memory leak.

There are various sources of fragmentation in Firefox, so unfortunately you aren't going to return to the same level of memory after closing all windows.

Comment 14

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

Dependency on bug 795579 has been fixed and Mozmill 1.5.19 released. So we can go ahead with a longer delay.

Comment 15

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.0

* I propose a 5s delay due to the following reasons:

* Compared results between 5s and 9s, there is no difference:
  9s result: http://mozmill-crowd.blargon7.com/#/endurance/report/d11b1de413a0179d904e73723068dc49
  5s result: http://mozmill-crowd.blargon7.com/#/endurance/report/d11b1de413a0179d904e7372300ccc18

* We cannot decrease under 5s because of http://mozmill-crowd.blargon7.com/#/endurance/report/d11b1de413a0179d904e73723013bbe4

* 5s is acceptable and we respect the 10s limit
* Dave said on iRC that we are ok to have endurance testrun last longer, it was initially by design

Comment 16

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 666921 [details] [diff] [review]
patch v1.0

Andrew, I would really appreciate your feedback on my proposal considering the investigation results. Thanks!

Comment 17

[Andrew McCreight [:mccr8]]

Comment on attachment 666921 [details] [diff] [review]
patch v1.0

Seems reasonable to me, but :johns would be a better person to look at this, as he's spent a fair amount of time doing similar investigations.

Comment 18

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

Comment on attachment 666921 [details] [diff] [review]
patch v1.0

Review of attachment 666921 [details] [diff] [review]:
-----------------------------------------------------------------

Patch-wise it looks fine. But lets get feedback from John before we check-in the change.

Comment 19

[John Schoenick [:johns]]

Comment on attachment 666921 [details] [diff] [review]
patch v1.0

Review of attachment 666921 [details] [diff] [review]:
-----------------------------------------------------------------

If I recall correctly there are some timers between GC and image throw-away that run longer than 5s, so this wont guarantee that everything that runs based on the wall clock will occur. However, if the idea is just to make the tests more realistic then it should be a big improvement.

Something this might not address is endurance tests that are doing too many actions per iteration -- For instance, if a test opens 30 tabs per iteration, we would want to somehow pause between tab loads as well.

FWIW areweslimyet has a short delay between actions (opening tabs) and a 30s delay between iterations, to ensure any idle-cleanup happens before measuring memory.

Comment 20

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to John Schoenick [:johns] from comment #19)
> Something this might not address is endurance tests that are doing too many
> actions per iteration -- For instance, if a test opens 30 tabs per
> iteration, we would want to somehow pause between tab loads as well.

The delay is between iterations and also entities, so we should have this covered.

Before we land this, we should ensure that we can afford the endurance tests to take ~85 minutes without preventing other testruns from executing. Also, we should communicate this change so that anyone monitoring the endurance charts is aware of the upcoming changes.

Comment 21

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

By later today we will have 3 VMs per platform. So I would say it should be possible by then. Also given that the endurance tests have a lower priority.

Comment 22

[Dave Hunt [:davehunt] [he/him] ⌚BST]

This was dependant on https://github.com/mozilla/mozmill-ci/issues/173 which is now resolved.

Comment 23

[Daniela Petrovici]

I checked the patch with the 5 seconds delay and it is still working properly. The report is here:
http://mozmill-crowd.blargon7.com/#/endurance/report/d96621f14ec6678200ea5c05eaf8e38c

Since we previously based the investigation on testTabbedBrowsing_OpenNewWindow/test1.js which is now skipped, I have used the patch from bug 800872 for testing this.

The endurance tests run for about 1.5 hours with the added delay.

I have also tried a smaller delay (4 seconds) and the tests are running in 1.10 hours. The report for the second trial is here: http://mozmill-crowd.blargon7.com/#/endurance/report/a83c700664548dba07298b74bf035197

Please tell me if I need to try something else too.

Comment 24

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 666921 [details] [diff] [review]
patch v1.0

Review of attachment 666921 [details] [diff] [review]:
-----------------------------------------------------------------

Landed as:
http://hg.mozilla.org/qa/mozmill-automation/rev/164b9e59bdf1