Closed Bug 863137 Opened 11 years ago Closed 11 years ago

Make gaia-ui-tests contributor friendly.

Categories

(Mozilla QA :: General, defect)

Product:
Mozilla QA
Component:
General
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: nbp, Unassigned)

References

Details

Currently the running gaia-ui-tests wipe the sdcard, remove the contacts, make phone calls, and so on … Without any warning.

The following bug goal is to ensure that the user *has been warned* by checking that a file exists on the sdcard before doing any destructive & costly actions.  If the file does not exists then the test suite should fail with a *HUGE* warning asking to add a dummy file on the sdcard.

I would even suggest to do a sleep with a warning message before each run if the file content does not contain a statement (check with a checksum) to ensure that the person has read a wiki page explaining that putting the special statement in the file mean that the phone would be dedicated to automated testing.

Doing so would prevent what just happened to me, which is using my data plan (from a foreign country), making phone calls, removing more than 3 years of photos, …

Contributors are likely to run tests and to trust Mozilla for respecting their data, doing such actions on their phone is far from being a proof of respect.  This is even more important as contributors are unlikely to have phones dedicated to run tests and are likely to use their own devices for running such tests.
I'm sorry to hear about your loss of data, and this is definitely something we should warn about. It's likely that this will be even more destructive soon as we work towards having the device in a 'clean' state for every test.

(In reply to Nicolas B. Pierron [:nbp] from comment #0)
> Currently the running gaia-ui-tests wipe the sdcard, remove the contacts,
> make phone calls, and so on … Without any warning.
> 
> The following bug goal is to ensure that the user *has been warned* by
> checking that a file exists on the sdcard before doing any destructive &
> costly actions.  If the file does not exists then the test suite should fail
> with a *HUGE* warning asking to add a dummy file on the sdcard.

Not all tests require an sdcard to be present, so I'm not sure this would be the best solution. Perhaps prompting for user input, or having a file present in the working directory would be suitable alternatives? In fact, we already use a test variables file, so perhaps we could make this required and include a key to indicate that the user has acknowledged this risk.

> Doing so would prevent what just happened to me, which is using my data plan
> (from a foreign country), making phone calls, removing more than 3 years of
> photos, …

How did you execute the tests? If you specify the tests directory all tests will be discovered and executed, however if you use the manifest file then you can filter tests by type (for example, exclude all carrier tests). We certainly need to improve this documentation, and even then it doesn't prevent your scenario from happening.

As I understand it, no phone calls should be made unless a phone number is specified in the test variables file, but all contacts/media/messages will be removed between tests, and the cell data will be activated for tests that require it (although most will prefer WiFi if it's available and specified in your test variables file).

> Contributors are likely to run tests and to trust Mozilla for respecting
> their data, doing such actions on their phone is far from being a proof of
> respect.  This is even more important as contributors are unlikely to have
> phones dedicated to run tests and are likely to use their own devices for
> running such tests.

These tests will only run on engineering builds or custom builds with Marionette enabled. As I understand it they will not run on dogfood builds. Again, this doesn't solve the problem for contributors using such enabled builds, and we need to do what we can to prevent such data loss.
Sorry Nicolas :((
I like the idea of a file in the root of the repo that the framework looks for, and if it isn't present (or perhaps is empty), then the user is prompted at the command line with a warning. We could easily create or populate this file in our Jenkins job to allow those to run without intervention.
(In reply to Dave Hunt (:davehunt) from comment #1)
> How did you execute the tests? If you specify the tests directory all tests
> will be discovered and executed, however if you use the manifest file then
> you can filter tests by type (for example, exclude all carrier tests). We
> certainly need to improve this documentation, and even then it doesn't
> prevent your scenario from happening.

Sorry, I only saw that the data connection had been enabled by the script, even if it was disable before.  I didn't saw any phone calls made by the gaia-ui-tests but knowing it was enabling the data connection, and that the marionette test suite with-in Gecko repository made a phone call on my behalf, so I just assumed that the gaia-ui-tests was likely to check that too.

Usually, I am running the tests with either a specified directory or a specified test, but even in these cases, the sdcard is wiped out.
As some tests already require use of a test variables file, I would propose we make this mandatory. We can add a key/value that indicates the user is aware of the risks. It would look something like:

{
  "acknowledged_risks": false
  "remote_phone_number": "...",
  ...
}

The template would include false by default, and we would exit with a suitable error message pointing the users to a wiki page or README that explains the risks and how to accept them. For our own CI we already have per-device test variable files, so we would just need to add this key/value to those files.
I like that Dave, nice and clean and using existing frameworks.

What do you think Nicolas?
(In reply to Zac C (:zac) from comment #6)
> I like that Dave, nice and clean and using existing frameworks.
> 
> What do you think Nicolas?

I'll suggest having 3 states for "acknowledge_risks", because flipping a boolean does not imply that you read the documentation (how many times did you click "yes, I read the damn license of 300 pages").

 - "acknowledged_risks": false
   Should display an error message telling the risk and suggest about reading the wiki page.

 - "acknowledged_risks": true
   Should display a warning with the risks, with a sleep of 30 seconds and suggesting to hit Ctrl-C to interrupt.

 - "acknowledged_risks": "This phone is dedicated to the test suite and there is nothing valuable on it, …"
   This secret message checked against a check-sum (not in the code) should be documented in the wiki and disable all errors & warnings.


The reason for this 3 states, is that somebody which use the test suite occasionally will still have a warning with a low-overhead, but somebody which use it daily can get rid of it by reading the documentation.
(In reply to Nicolas B. Pierron [:nbp] from comment #7)
>  - "acknowledged_risks": "This phone is dedicated to the test suite and
> there is nothing valuable on it, …"
>    This secret message checked against a check-sum (not in the code) should
> be documented in the wiki and disable all errors & warnings.

The message should not be in the code, but only on the wiki page.  The checksum should be in the code to compare it against the message.
(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> (In reply to Zac C (:zac) from comment #6)
> I'll suggest having 3 states for "acknowledge_risks", because flipping a
> boolean does not imply that you read the documentation (how many times did
> you click "yes, I read the damn license of 300 pages").

The warning won't be 300 pages, and to increase the likelihood that it's read we could even remove the default key/value from the template and provide the details in the wiki/documentation. That way, it must be at least referenced.

I think having the 30 second sleep is a reasonable suggestion, but perhaps we could implement an additional (undocumented, except in code) 'skip_warning' key rather than using a secret message and check-sum?

I think if someone adds the key to indicate they've acknowledged the risks _and_ the key to skip the warning, then they're well aware of the potential data loss and costs.
(In reply to Dave Hunt (:davehunt) from comment #9)
> I think if someone adds the key to indicate they've acknowledged the risks
> _and_ the key to skip the warning, then they're well aware of the potential
> data loss and costs.

Sounds good for me. :)
I have opened a pull request. Please feel free to provide feedback: https://github.com/mozilla/gaia-ui-tests/pull/717
Fixed in gaiatest 0.10
https://pypi.python.org/pypi/gaiatest/0.10
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.