Closed
Bug 984340
Opened 11 years ago
Closed 10 years ago
Enable sdcard tests on b2g desktop
Categories
(Firefox OS Graveyard :: Gaia::UI Tests, defect, P2)
Firefox OS Graveyard
Gaia::UI Tests
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: davehunt, Assigned: davehunt)
References
Details
Attachments
(1 file, 1 obsolete file)
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.
Updated•11 years ago
|
Assignee: nobody → vivekb.balakrishnan
Comment 3•11 years ago
|
||
Hi Vivek, how can we help you? Do you need help getting started?
Flags: needinfo?(vivekb.balakrishnan)
Comment 4•11 years ago
|
||
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.
Flags: needinfo?(vivekb.balakrishnan)
Comment 5•11 years ago
|
||
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.
Assignee: vivekb.balakrishnan → nobody
Updated•11 years ago
|
Priority: -- → P2
Comment 6•11 years ago
|
||
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.
Attachment #8420999 -
Flags: feedback?(viorela.ioia)
Attachment #8420999 -
Flags: feedback?(florin.strugariu)
Attachment #8420999 -
Flags: feedback?(dave.hunt)
Attachment #8420999 -
Flags: feedback?(bob.silverberg)
Updated•11 years ago
|
Whiteboard: [mentor=davehunt][lang=py]
Comment 7•11 years ago
|
||
Comment on attachment 8420999 [details]
github pr
Upgrading to r? , all outstanding issues resolved. Go mad on it! :)
Attachment #8420999 -
Flags: review?(viorela.ioia)
Attachment #8420999 -
Flags: review?(florin.strugariu)
Attachment #8420999 -
Flags: review?(dave.hunt)
Attachment #8420999 -
Flags: review?(bob.silverberg)
Attachment #8420999 -
Flags: feedback?(viorela.ioia)
Attachment #8420999 -
Flags: feedback?(florin.strugariu)
Attachment #8420999 -
Flags: feedback?(dave.hunt)
Attachment #8420999 -
Flags: feedback?(bob.silverberg)
Assignee | ||
Comment 8•11 years ago
|
||
Comment on attachment 8420999 [details]
github pr
Excited to see this extra coverage. See pull for comments.
Attachment #8420999 -
Flags: review?(dave.hunt) → review-
Comment 9•11 years ago
|
||
Comment on attachment 8420999 [details]
github pr
Fixed the logic inside mkDirs,
Responded to some comments inside the PR.
Attachment #8420999 -
Flags: review- → review?(dave.hunt)
Assignee | ||
Comment 10•11 years ago
|
||
Comment on attachment 8420999 [details]
github pr
I'll wait to give another review until you have some feedback from others.
Attachment #8420999 -
Flags: review?(dave.hunt)
Comment 11•11 years ago
|
||
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•11 years ago
|
||
Comment on attachment 8420999 [details]
github pr
I am seeing one consistent failure when running the tests. I also added a couple of comments/questions into the PR.
Attachment #8420999 -
Flags: review?(bob.silverberg) → review-
Comment 13•11 years ago
|
||
Comment on attachment 8420999 [details]
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.
Attachment #8420999 -
Flags: review- → review?(bob.silverberg)
Comment 14•11 years ago
|
||
Comment on attachment 8420999 [details]
github pr
Tests enabled on desktop b2g are passing locally
Attachment #8420999 -
Flags: review?(viorela.ioia) → review+
Comment 15•11 years ago
|
||
Comment on attachment 8420999 [details]
github pr
One last comment/question from me in the PR.
Attachment #8420999 -
Flags: review?(bob.silverberg) → review-
Updated•11 years ago
|
Attachment #8420999 -
Flags: review- → review+
Updated•11 years ago
|
Attachment #8420999 -
Flags: review?(florin.strugariu) → review+
Comment 16•11 years ago
|
||
Comment on attachment 8420999 [details]
github pr
another r? for dave as requested.
Attachment #8420999 -
Flags: review?(dave.hunt)
Assignee | ||
Comment 17•11 years ago
|
||
Comment on attachment 8420999 [details]
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.
Attachment #8420999 -
Flags: review?(dave.hunt) → review-
Comment 18•11 years ago
|
||
Comment on attachment 8420999 [details]
github pr
I made a few changes to the copy feature and responded to your comments.
Attachment #8420999 -
Flags: review- → review?(dave.hunt)
Comment 20•11 years ago
|
||
Comment on attachment 8420999 [details]
github pr
Bob, can I get a final review from you? Cheers
Attachment #8420999 -
Flags: review?(dave.hunt)
Attachment #8420999 -
Flags: review?(bob.silverberg)
Attachment #8420999 -
Flags: review+
Updated•11 years ago
|
Attachment #8420999 -
Attachment is obsolete: true
Attachment #8420999 -
Flags: review?(bob.silverberg)
Assignee | ||
Comment 22•11 years ago
|
||
I think this is really close. I'll take it and come up with a new patch.
Assignee: nobody → dave.hunt
Status: NEW → ASSIGNED
Assignee | ||
Comment 23•10 years ago
|
||
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?
Attachment #8426477 -
Flags: feedback?(wlachance)
Comment 24•10 years ago
|
||
Comment on attachment 8426477 [details] 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
Attachment #8426477 -
Flags: feedback?(wlachance) → feedback-
Assignee | ||
Comment 25•10 years ago
|
||
(In reply to William Lachance (:wlach) from comment #24) > Comment on attachment 8426477 [details] > 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.
Assignee | ||
Comment 26•10 years ago
|
||
Comment on attachment 8426477 [details] 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.
Attachment #8426477 -
Flags: feedback- → feedback?(wlachance)
Comment 27•10 years ago
|
||
Comment on attachment 8426477 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19497 Yes, this makes much more sense to me.
Attachment #8426477 -
Flags: feedback?(wlachance) → feedback+
Assignee | ||
Comment 28•10 years ago
|
||
Comment on attachment 8426477 [details] 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?
Attachment #8426477 -
Flags: feedback+ → feedback?(wlachance)
Comment 29•10 years ago
|
||
Comment on attachment 8426477 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19497 Looks good! I left a few comments inside the PR
Attachment #8426477 -
Flags: feedback?(wlachance) → feedback+
Assignee | ||
Comment 30•10 years ago
|
||
Comment on attachment 8426477 [details] 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.
Attachment #8426477 -
Flags: review?(bob.silverberg)
Attachment #8426477 -
Flags: feedback?(rwood)
Assignee | ||
Comment 31•10 years ago
|
||
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•10 years ago
|
||
Comment on attachment 8426477 [details] 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.
Attachment #8426477 -
Flags: review-
Assignee | ||
Comment 33•10 years ago
|
||
Comment on attachment 8426477 [details] 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.
Attachment #8426477 -
Flags: review?(bob.silverberg)
Attachment #8426477 -
Flags: feedback?(rwood)
Assignee | ||
Comment 34•10 years ago
|
||
Comment on attachment 8426477 [details] 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/
Attachment #8426477 -
Flags: review?(zcampbell)
Attachment #8426477 -
Flags: review?(bob.silverberg)
Attachment #8426477 -
Flags: review-
Attachment #8426477 -
Flags: feedback?(rwood)
Assignee | ||
Comment 35•10 years ago
|
||
Comment on attachment 8426477 [details] 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.
Attachment #8426477 -
Flags: review?(zcampbell)
Attachment #8426477 -
Flags: review?(bob.silverberg)
Attachment #8426477 -
Flags: feedback?(rwood)
Flags: needinfo?(dave.hunt)
Comment 36•10 years ago
|
||
Comment on attachment 8426477 [details] 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?
Attachment #8426477 -
Flags: feedback+
Assignee | ||
Comment 37•10 years ago
|
||
(In reply to Robert Wood [:rwood] from comment #36) > Comment on attachment 8426477 [details] > 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.
Assignee | ||
Comment 38•10 years ago
|
||
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.
Assignee | ||
Comment 39•10 years ago
|
||
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/
Assignee | ||
Comment 40•10 years ago
|
||
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.
Depends on: 1021767
Flags: needinfo?(dave.hunt)
Assignee | ||
Comment 41•10 years ago
|
||
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/
Depends on: 1023199
Assignee | ||
Comment 42•10 years ago
|
||
Comment on attachment 8426477 [details] 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.
Attachment #8426477 -
Flags: review?(zcampbell)
Comment 43•10 years ago
|
||
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•10 years ago
|
||
Comment on attachment 8426477 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19497 phew, a stable run! All failures can be accounted for. r+
Attachment #8426477 -
Flags: review?(zcampbell) → review+
Comment 45•10 years ago
|
||
Merged on master: https://github.com/mozilla-b2g/gaia/commit/3f13527bfd848ae553dd122acf6aed4fac936651 Thanks for your patience with this one Dave.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•