984340 - Enable sdcard tests on b2g desktop

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::UI Tests, defect, P2)

Description

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

Now that bug 959591 is resolved, we should enable the sdcard tests for b2g desktop. I've tested this locally and it works well. We'll probably need to do the following:

1. If the target is a desktop build, create a temporary directory in setUp for the sdcard contents, and set the 'device.storage.overrideRootDir' preference to the path.
2. Modify push_resource in gaia_test.py to copy the resource into the temporary directory if the target is b2g desktop.
3. Remove the temporary directory in tearDown.
4. Enhance cleanup_sdcard in gaia_test so the files are removed if the target is b2g desktop.

The test_cleanup_sdcard.py unit test should be a good indication of this working on b2g desktop.

This should allow us to enable an additional ~20 tests on b2g desktop.

Comment 1

[Zac C (:zac)]

Good news :)

Comment 2

[Vivek Balakrishnan[:vivek]]

I would like to work on this.

Comment 3

[Zac C (:zac)]

Hi Vivek, how can we help you? Do you need help getting started?

Comment 4

[Vivek Balakrishnan[:vivek]]

Hi Zac, This is my first bug in b2g UI tests. I have compiled a working b2g desktop build. Can you give me some pointers on gaia UI tests.

Comment 5

[Zac C (:zac)]

Vivek I emailed you about getting up and going. We'll assign you a task when you're familiar and comfortable with the test suite.

Comment 6

[Zac C (:zac)]

Attachment: github pr

NB this is not a final cut, there are still a few things to work out, particularly wrt which tests I can enable and which I cannot.

Just putting it up for r? or f? to get 1st-iteration feedback.

Comment 7

[Zac C (:zac)]

Comment on attachment 8420999 [details] [review]
github pr

Upgrading to r? , all outstanding issues resolved. Go mad on it! :)

Comment 8

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

Comment on attachment 8420999 [details] [review]
github pr

Excited to see this extra coverage. See pull for comments.

Comment 9

[Zac C (:zac)]

Comment on attachment 8420999 [details] [review]
github pr

Fixed the logic inside mkDirs,
Responded to some comments inside the PR.

Comment 10

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

Comment on attachment 8420999 [details] [review]
github pr

I'll wait to give another review until you have some feedback from others.

Comment 11

[Zac C (:zac)]

Adhoc:
http://selenium.qa.mtv2.mozilla.com:8080/job/b2g.hamachi.mozilla-central.ui.adhoc/397/

Gaia Try:
https://tbpl.mozilla.org/?tree=Gaia-Try&rev=96ec42660e19

Comment 12

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8420999 [details] [review]
github pr

I am seeing one consistent failure when running the tests. I also added a couple of comments/questions into the PR.

Comment 13

[Zac C (:zac)]

Comment on attachment 8420999 [details] [review]
github pr

Bob I made a few of the changes you suggested, see the recent commit.. 
Also for the test_settings_wallpaper, use --timeout 30000 on the command line. I guess the default is a bit short for this step and we should override it in the test itself.

Comment 14

[Viorela Ioia [:viorela]]

Comment on attachment 8420999 [details] [review]
github pr

Tests enabled on desktop b2g are passing locally

Comment 15

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8420999 [details] [review]
github pr

One last comment/question from me in the PR.

Comment 16

[Zac C (:zac)]

Comment on attachment 8420999 [details] [review]
github pr

another r? for dave as requested.

Comment 17

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

Comment on attachment 8420999 [details] [review]
github pr

There are still issues with this patch, but it's looking better. I'm concerned that these issues were not picked up in previous reviews, could you please ensure to include me as a reviewer on the next iteration? Thanks.

Comment 18

[Zac C (:zac)]

Comment on attachment 8420999 [details] [review]
github pr

I made a few changes to the copy feature and responded to your comments.

Comment 19

[Zac C (:zac)]

Made another commit with README.md content and separator change.

Comment 20

[Zac C (:zac)]

Comment on attachment 8420999 [details] [review]
github pr

Bob, can I get a final review from you? Cheers

Comment 21

[Zac C (:zac)]

Not working on this anymore.

Comment 22

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

I think this is really close. I'll take it and come up with a new patch.

Comment 23

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

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19497

Note that this is *not* ready for review/merge. At this point I am just requesting feedback. It needs more tests, and I need to run tests across multiple platforms.

Will: Please take a look over this patch. It introduces a file manager as a file based simulated device manager. I want to write some tests for this, and was wondering if there's even room for this in mozdevice. What do you think?

Comment 24

[William Lachance (:wlach)]

Comment on attachment 8426477 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19497

Hmmm.... I feel like a bette solution here would be to add some kind of class for doing the test setup/teardown that calls either mozdevice or native filesystem calls depending on the exact use case.

My experience is that these kind of "fake" classes often cause confusion, since you need to create a mental model of the thing that's being faked in your head and then map that to what's actually happening. An example of this is the "Process" class in the mochitest remote tests, which is not actually a process:

http://mxr.mozilla.org/mozilla-central/source/build/mobile/remoteautomation.py#187

Comment 25

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

(In reply to William Lachance (:wlach) from comment #24)
> Comment on attachment 8426477 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19497
> 
> Hmmm.... I feel like a bette solution here would be to add some kind of
> class for doing the test setup/teardown that calls either mozdevice or
> native filesystem calls depending on the exact use case.
> 
> My experience is that these kind of "fake" classes often cause confusion,
> since you need to create a mental model of the thing that's being faked in
> your head and then map that to what's actually happening. An example of this
> is the "Process" class in the mochitest remote tests, which is not actually
> a process:
> 
> http://mxr.mozilla.org/mozilla-central/source/build/mobile/remoteautomation.
> py#187

Thanks Will. You're right in that it's a fake device in this case, but we could turn it around and say that devicemanager implements a filemanager. That said, such a think wouldn't neatly fit into mozdevice, and yet it would be tightly coupled with it. I'm not sure we can solve this simply in the setup/teardown, but I'll take another stab at this and see if I can remove the need for the FileManager class.

Comment 26

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

Comment on attachment 8426477 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19497

Will: Could you take another look at this? I'm much happier with this approach, and it feels like a bit of a cleanup. Packages that depend on gaiatest and use any of these methods are going to need to be updated (I think it's just b2gpopulate) but this actually enhances their support for desktop B2G too.

There are two parts I'm not mad. One is the way we pass a device manager to GaiaDevice. I'd prefer to create this within the object, but we already have one via the B2GTestCaseMixin. The other part is having to reset the storage override preference when we restart our B2G instance, but I don't think there's a way to avoid this.

Comment 27

[William Lachance (:wlach)]

Comment on attachment 8426477 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19497

Yes, this makes much more sense to me.

Comment 28

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

Comment on attachment 8426477 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19497

Thanks for the previous feedback Will. I've cleaned this up and also taken your suggestion regarding inheritance. Could you take another look and see if this is what you had in mind?

Comment 29

[William Lachance (:wlach)]

Comment on attachment 8426477 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19497

Looks good! I left a few comments inside the PR

Comment 30

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

Comment on attachment 8426477 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19497

Okay, I think this is ready for review. I'm also interested to see how the Travis-CI run goes. It's very possible that enabling these tests will introduce a few intermittent failures due to the speed that desktop B2G executes.

Rob: Would you mind taking a look over the endurance changes? These are mostly just the removal of a superfluous path when pushing resources to the device.

Comment 31

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

I've also triggered an adhoc job: http://selenium.qa.mtv2.mozilla.com:8080/view/B2G%20Flame/job/b2g.flame.mozilla-central.ui.adhoc/7/

Comment 32

[Zac C (:zac)]

Comment on attachment 8426477 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19497

r-, the syntax for the test skip in manifest.ini is wrong and also I think skips on device. it needs to skip only where os == linux64 *and* device == "desktop" to target the desktopb2g CI environments.

Comment 33

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

Comment on attachment 8426477 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19497

I'm confident that the adhoc failures are unrelated. There was a Travis-CI failure that's likely to be an intermittent though. I'll take a look at that, and also address Zac's comments. Removing review/feedback requests for now.

Comment 34

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

Comment on attachment 8426477 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19497

I've hopefully addressed the issues. Let's see how the Travis-CI job likes it. I've also triggered a Jenkins adhoc build: http://selenium.qa.mtv2.mozilla.com:8080/view/B2G Flame/job/b2g.flame.mozilla-central.ui.adhoc/14/

Comment 35

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

Comment on attachment 8426477 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19497

It looks like the manifest skips aren't working on Travis so I'll need to look into this. Also, the flick test failed again.

Comment 36

[Robert Wood [:rwood]]

Comment on attachment 8426477 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19497

This looks really cool!

One question, when running more than one instance of b2g desktop on the same machine, how do you prevent the storage_path on both instances from pointing to the same local file system location? 

From a quick look at tempfile.mkdtemp(), it looks like it will use an TMP env var? So will you have Jenkins set a TMP env var for each build perhaps?

Comment 37

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

(In reply to Robert Wood [:rwood] from comment #36)
> Comment on attachment 8426477 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19497
> 
> This looks really cool!
> 
> One question, when running more than one instance of b2g desktop on the same
> machine, how do you prevent the storage_path on both instances from pointing
> to the same local file system location? 
> 
> From a quick look at tempfile.mkdtemp(), it looks like it will use an TMP
> env var? So will you have Jenkins set a TMP env var for each build perhaps?

Thanks for the feedback, Rob! The tempfile.mkdtmp() creates a new temporary directory for every test, and then this is removed in tearDown. If tests are running in parallel (something which I'm not actually sure has been done yet) then each call to mkdtmp() should still return a unique directory. The temporary directory is only used for B2G desktop, as devices will use their internal/external storage as needed. On Jenkins we're only running tests against devices.

Comment 38

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

Okay, so the manifest skips were invalid. I've updated them to skip if the OS is 'linux' and the device is 'desktop' (for bug 1009114). If we need to skip based on 32/64bit then the syntax might be:

> skip-if = os == "linux" && bits == 64 && device == "desktop"

I also spotted another issue when running on Gaia-Try where we might try switching to the displayed app while B2G is still starting up, so I've added an extra wait when we create a GaiaDevice object for B2G to be ready. Once I have a clean Gaia-Try or Travis run I'll re-request review.

Comment 39

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

I've also triggered an adhoc build on Jenkins: http://selenium.qa.mtv2.mozilla.com:8080/view/B2G%20Flame/job/b2g.flame.mozilla-central.ui.adhoc/18/

Comment 40

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

So TBPL was happy, but Jenkins wasn't. The wait for B2G to be ready wasn't working when the device is locked, which happens between the flashing and tests running on Jenkins. I also realised that such a wait could cause every test to timeout if the device gets itself in a bad state. Zac raised bug 1021767, which would allow us to set preferences without B2G being ready to interact with, which is what we're trying to do here. Let's wait for that to land.

Comment 41

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

The gallery flick improvement has been split out into bug 1023199 so I've rebased and removed it from this patch. Running the Gaia-Try[1], Travis-CI[2], and adhoc[3] jobs to see how this is looking now.

[1] https://tbpl.mozilla.org/?tree=Gaia-Try&rev=6e49abf786ee
[2] https://travis-ci.org/mozilla-b2g/gaia/builds/27218474
[3] http://selenium.qa.mtv2.mozilla.com:8080/view/B2G%20Flame/job/b2g.flame.mozilla-central.ui.adhoc/24/

Comment 42

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

Comment on attachment 8426477 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19497

Okay, let's get this reviewed. The last adhoc build was suffering from an unrelated regression in the builds. The last Gaia-Try and Travis-CI builds were successful.

Here are the builds for the latest patch:
Gaia-Try: https://tbpl.mozilla.org/?tree=Gaia-Try&rev=9ce5591d350a
Travis-CI: https://travis-ci.org/mozilla-b2g/gaia/builds/27389689
Jenkins: http://selenium.qa.mtv2.mozilla.com:8080/view/B2G%20Flame/job/b2g.flame.mozilla-central.ui.adhoc/25/

I believe Bob is now on PTO, so would you mind taking this review Zac? If it looks good and there are no issues with the above builds, please go ahead and land it.

Comment 43

[Zac C (:zac)]

The builds were a bit flaky and I don't think they were related to this patch but I'm going to wait for it to stabilise (probably tomorrow) before I can review properly.

Comment 44

[Zac C (:zac)]

Comment on attachment 8426477 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19497

phew, a stable run!

All failures can be accounted for.

r+

Comment 45

[Zac C (:zac)]

Merged on master:
https://github.com/mozilla-b2g/gaia/commit/3f13527bfd848ae553dd122acf6aed4fac936651

Thanks for your patience with this one Dave.