Closed Bug 915572 Opened 11 years ago Closed 11 years ago

For gaia endurance test cases: prevent a reset when a test case is completed and move to next test case

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox28 fixed, b2g-v1.2 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox28 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: mdas, Assigned: davehunt)

References

Details

Attachments

(1 file, 1 obsolete file)

Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13911
358 bytes, text/html
zcampbell
: review+
wachen
: review+
Details 
This is needed by the Taipei team running MTBF endurance tests.
Could you provide more details? The restart should only happen between tests when the --restart command line argument is provided.
Bruce wants data on the device to persist so it means avoiding cleanUp.
Ah, I confused reset with restart. We should separate these, so they can be individually toggled. Currently we always cleanup some items (contacts, settings, etc), and only cleanup others (indexeddb, profile, etc) when we perform a restart.

I would suggest by default that we cleanup and restart, and add the command line options --no-cleanup and --no-restart. This would be a change of the restart behaviour, which is currently off by default.

This won't prevent any cleanup that the tests themselves may perform, so the tests may need tweaking or perhaps need to have the cleanup boolean available to them from the test runner.

How does that sound?
I thought that Bruce meant to prevent the restart between tests, which can be resolved by removing the --restart command if that's how they're running tests, but now I'm not so sure. Bruce, can you clarify this requirement, and check if Dave's solution in Comment #3 is suitable?
Flags: needinfo?(bweng)
I agree with Dave that add --no-cleanup to skip cleanUp() method so that we can keep contacts, settings and so on.
However, I think we don't need to replace --restart option by --no-restart option.
Like the following table.
 ---------------------------------------------
  actions  | default |  options
 ----------+---------+------------------------
  restart  | False   | --restart for True
  cleanup  | True    | --no-cleanup for False
 ---------------------------------------------
(In reply to Askeing Yen[:askeing] from comment #5)
> I agree with Dave that add --no-cleanup to skip cleanUp() method so that we
> can keep contacts, settings and so on.
> However, I think we don't need to replace --restart option by --no-restart
> option.
> Like the following table.
>  ---------------------------------------------
>   actions  | default |  options
>  ----------+---------+------------------------
>   restart  | False   | --restart for True
>   cleanup  | True    | --no-cleanup for False
>  ---------------------------------------------
for example
https://github.com/askeing/gaia-ui-tests/commit/d4a1b76ba8784332bf044c04e1123d2978910cd1
The reason for suggesting that restart became default is that more and more of the UI tests are relying on it. Perhaps switching that is a separate issue though. I'll take this issue and add a --no-cleanup option similar to the one shown in comment 6.
(In reply to Malini Das [:mdas] from comment #4)
> I thought that Bruce meant to prevent the restart between tests, which can
> be resolved by removing the --restart command if that's how they're running
> tests, but now I'm not so sure. Bruce, can you clarify this requirement, and
> check if Dave's solution in Comment #3 is suitable?

Hi Al,

Could you please provide more information about what Malini wanted? 
Thanks.
Bruce
Flags: needinfo?(bweng) → needinfo?(atsai)
(In reply to bweng from comment #8)
> (In reply to Malini Das [:mdas] from comment #4)
> > I thought that Bruce meant to prevent the restart between tests, which can
> > be resolved by removing the --restart command if that's how they're running
> > tests, but now I'm not so sure. Bruce, can you clarify this requirement, and
> > check if Dave's solution in Comment #3 is suitable?
> 
> Hi Al,
> 
> Could you please provide more information about what Malini wanted? 
> Thanks.
> Bruce

They catch the meaning correctly. For the test we have now, we should still set the restart option as default true. I have no idea about what further information I can provide at this point.
Flags: needinfo?(atsai)
Depends on: 916023
We're preparing to move the functional UI tests to the Gaia repository today, so let's hold off on this until that's done. We can then land this directly in the Gaia repository and release a new version of gaiatest.
If we skip the entire cleanup method then it's not just data that will be left behind.. the device may be left locked, not on the home screen, and with settings that interfere with the smooth running of tests. Whatever tests you write that make use of this should be able to cope with this unknown state that the device may be in.
Hi Al, could you confirm that comment 12 indicates desired behaviour? By skipping cleanup we will be doing nothing to ensure the device is in a clean state.
Assignee: nobody → dave.hunt
Flags: needinfo?(atsai)
Yes. In my mind, we need to back to homescreen everytime (or lock the screen if needed) after each testing in TearDown part. I heard that for some reasons we don't have teardown step for testing now. If that's ture, I believe we need to make sure that all the tests we run with the no-clean parameter will go back to the homescreen or locked screen for keeping the tests running well.
Flags: needinfo?(atsai)
(In reply to Al Tsai [:atsai] from comment #14)
> Yes. In my mind, we need to back to homescreen everytime (or lock the screen
> if needed) after each testing in TearDown part. I heard that for some
> reasons we don't have teardown step for testing now. If that's ture, I
> believe we need to make sure that all the tests we run with the no-clean
> parameter will go back to the homescreen or locked screen for keeping the
> tests running well.

We do have a base tearDown and tests can add their own tearDown steps, however if setUp fails for a test then tearDown will not be run. Our current cleanUp is performed as part of setUp.

You can see everything we do as cleanUp here: https://github.com/mozilla-b2g/gaia/blob/9c418e409b50f4d5526cefd70fecefdd6b4c7c23/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L455

Here's a summary:
* Remove media files
* Enable device radio
* Disable passcode lock
* Reset language to English
* Disable Spanish keyboard
* Disable keyboard first time usage screen
* Reset timezone to PST
* Reset Do Not Track to default value
* Restore any settings defined in test variables
* Unlock the device
* Kill all running apps
* Mute all audio channels
* Disable carrier data connection
* Disable cell roaming
* Forget all known networks
* Disable wifi
* Remove all contacts
* Return to home screen

We also currently remove the persistent storage and profile when we restart B2G.

Could you indicate which of the above you want to optionally be able to avoid between tests?
Flags: needinfo?(atsai)
Hi, Dave,
I think just only keep:
* Unlock the device
* Return to home screen
is fine as for now.
I agree with Walter that we will need to keep unlock the device and return to homescreen. The reset of them will be implemented in the teardown phase in each test cases if needed.
Flags: needinfo?(atsai)
Given that you still want some clean up items, I would suggest the following solution:

* Move the device cleanup (removing persistent storage and profile) to a method named cleanup_device which is called in setUp only when the device will be restarted and while b2g is stopped.
* Rename the cleanUp method to cleanup_gaia

Then you should be able to extend GaiaTestCase and override both of these methods. You can make cleanup_device do nothing, and make cleanup_gaia just unlock and return to homescreen.

How does that sound?
Flags: needinfo?(atsai)
It sounds perfect to me :)
Thanks a lot, Dave.
Flags: needinfo?(atsai)
This patch adds cleanup_data, cleanup_gaia, and cleanup_sdcard. They can all be overridden on the test class. This also allows us to run the cleanup_gaia test on desktop (Travis and TBPL).

The removal of media has been simplified by simply removing all files on the SD Card. This has the advantage of not needing to use Gaia or Gecko and should be considerably faster than enumerating the files and removing them one by one.
Comment on attachment 821562 [details] [diff] [review]
Allow data/gaia/sdcard cleanup to be overridden by tests. v1.0

Just requesting feedback for now, as I've yet to kick off a full run on my Hamachi. Let me know if this approach works for you and the MTBF tests.
Attachment #821562 - Flags: feedback?(atsai)
Would you mind to check it?
Flags: needinfo?(wachen)
Hi, Dave, 

The patch looks good for now. I am thinking about looking into it further later since I am not used to look ar a diff file like this :P

Also, please note that this CI job failed after started :P
Looks like I missed the --type argument in the adhoc run. I've added it to the default value now so this shouldn't happen again. New build is:

http://qa-selenium.mv.mozilla.com:8080/view/B2G%20Hamachi/job/b2g.hamachi.mozilla-central.ui.adhoc/12/
Flags: needinfo?(wachen)
Hi, Dave, I think this still looks good!
The test failed. Was something missing?
Sorry, I haven't had time to look into the failures. Please feel free to take this item from me, otherwise I will get to it as soon as I can.
Okay, the issue was related to the cleanup_gaia method being closely tied to the restart in setUp. I have changed it so we can call it between tests and it will perform a full reset unless otherwise specified.

New adhoc job:
http://qa-selenium.mv.mozilla.com:8080/view/B2G%20Hamachi/job/b2g.hamachi.mozilla-central.ui.adhoc/30/console

If the results of the above are comparable to without the patch applied then I'll submit a patch for review.
Flags: needinfo?(dave.hunt)
There were a number of errors when attempting to enable WiFi. I'll continue to investigate.
Flags: needinfo?(dave.hunt)
Interestingly, running the test_cleanup_gaia.py in isolation was fine. I've triggered another adhoc build for the full suite: http://qa-selenium.mv.mozilla.com:8080/view/B2G%20Hamachi/job/b2g.hamachi.mozilla-central.ui.adhoc/33
I tried replicating the failures locally without success. As they were related to WiFi, and we've recently fixed bug 921620, I'm running them again: http://qa-selenium.mv.mozilla.com:8080/view/B2G%20Hamachi/job/b2g.hamachi.mozilla-central.ui.adhoc/39

It looks much better so far...
Flags: needinfo?(dave.hunt)
There still seem to be some issue with this patch. I'm going to spend some more time looking into them.
Latest adhoc build has 9 failures. Zac, could you check if these are known intermittents? None of them appear to be related to the changes.
Flags: needinfo?(dave.hunt) → needinfo?(zcampbell)
They look pretty normal except for one:
test_settings_media_storage.py
Flags: needinfo?(zcampbell)
(In reply to Zac C (:zac) from comment #36)
> They look pretty normal except for one:
> test_settings_media_storage.py

I don't see that failure. Sorry, I should have linked to the specific adhoc build: http://qa-selenium.mv.mozilla.com:8080/view/B2G%20Hamachi/job/b2g.hamachi.mozilla-central.ui.adhoc/50/?
Flags: needinfo?(zcampbell)
Yep that one looks better!
All of those are known failures for when that job was run.
Flags: needinfo?(zcampbell)
Great, I'll file a pull request and request review.
Attachment #8336085 - Flags: review?(zcampbell)
Attachment #8336085 - Flags: review?(wachen)
Status: NEW → ASSIGNED
Comment on attachment 8336085 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13911

This looks good to me. PEP8 checks are clean. r+
If Zac can run it once to see if it works good, it should be fine
Attachment #8336085 - Flags: review?(wachen) → review+
Attachment #821562 - Attachment is obsolete: true
Attachment #821562 - Flags: feedback?(atsai)
I wish we had been a bit more aggressive and removed all of the `full_reset` block altogether and pushed this responsibility back to the users of gaiatest and/or the test setUp itself.
Leaving the device in the state that the test left it would be quite inline with not using more command line settings.

Most of this can easily be set without modifying code just by using the testvars.json settings{} block which is parsed outside of the cleanUp. so i a sense it offers more flexibility to whoever is the user of gaiatest.

It also stops this block of code from rotting if the default prefs change or become invalid.
Comment on attachment 8336085 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13911

Aside from c#42, this functionally works fine. r+
Attachment #8336085 - Flags: review?(zcampbell) → review+
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: