If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Use expected conditions where appropriate and optimise element lookups

RESOLVED FIXED

Status

Firefox OS
Gaia::UI Tests
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: davehunt, Assigned: davehunt, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(24 attachments, 2 obsolete attachments)

46 bytes, text/x-github-pull-request
Bebe
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Bebe
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Bebe
: review+
Details | Review | Splinter Review
52 bytes, text/plain
Bebe
: review+
Details
46 bytes, text/x-github-pull-request
Bebe
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Bebe
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Bebe
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Bebe
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Bebe
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Bebe
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Bebe
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Bebe
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Bebe
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Bebe
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Bebe
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Bebe
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Bebe
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Bebe
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Bebe
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
davehunt
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Martijn Wargers (dead)
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Martijn Wargers (dead)
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Martijn Wargers (dead)
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Martijn Wargers (dead)
: review+
Details | Review | Splinter Review
(Assignee)

Description

3 years ago
We have expected conditions, which can make the syntax for a Marionette wait easier to parse. Alongside this change we should optimise our current element lookups as demonstrated below. This is a low priority task, and it may be worth waiting for bug 1015242 to be addressed.

Current syntax waiting for an element to be displayed:

Wait(m, ignored_exceptions=NoSuchElementException).until(m.find_element(by, locator).is_displayed())

Syntax with lookup optimised and expected conditions:

el = Wait(m).until(expected.element_present(by, locator))
Wait(m).until(expected.element_displayed(element))

The first example will repeatedly lookup the element even after it's been found while it checks the visibility. The second will store the element once it's discovered and then only check the element for visibility without further lookups.
QA Whiteboard: [fxosqa-auto-backlog+]
(Assignee)

Comment 1

3 years ago
Created attachment 8516638 [details] [review]
Camera

Doing this in one go will be a large amount of work and will require a large review and testing effort. Let's take each app object as a separate patch. This patch is for the Camera app object. The final patch should be able to remove the majority of wait_for_ methods in the test class and base page class.

I did a bit of analysis over the changes in this patch. This reduces the number of method calls by 12,337 (~4%) and specifically reduces the number of times we call find_element by ~14%. While the impact on total time is difficult to measure, it's likely to be a positive (if negligible) effect.

Note also that we'll need to ensure future patches use the Wait class and expected conditions so that we don't revert to the less optimal approach currently in place.
Assignee: nobody → dave.hunt
Status: NEW → ASSIGNED
Attachment #8516638 - Flags: review?(florin.strugariu)
(Assignee)

Comment 2

3 years ago
Created attachment 8516683 [details] [review]
Calendar
Attachment #8516683 - Flags: review?(florin.strugariu)
(Assignee)

Comment 3

3 years ago
Created attachment 8516975 [details] [review]
Clock
Attachment #8516975 - Flags: review?(florin.strugariu)

Updated

3 years ago
Duplicate of this bug: 1009099
Attachment #8516638 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8516638 [details] [review]
Camera

https://github.com/mozilla-b2g/gaia/commit/c704e65f50870cbe49edd080312f86ca851c7700
Attachment #8516638 - Attachment is obsolete: true
Attachment #8516683 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8516683 [details] [review]
Calendar

https://github.com/mozilla-b2g/gaia/commit/ea9438b0ea56c14318c2873dc9e8ccccdaf2c994
Attachment #8516683 - Attachment is obsolete: true
Attachment #8516975 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8516975 [details] [review]
Clock

ate my gaia repo a couple of hours ago
Attachment #8516975 - Attachment is obsolete: true
Comment on attachment 8516975 [details] [review]
Clock

Wrong copy/paste Sorry

https://github.com/mozilla-b2g/gaia/commit/bf8d513adab5c5bbd9ddd144cfdfd2aca3858544
(Assignee)

Comment 9

3 years ago
Created attachment 8520641 [details]
Contacts

I haven't been able to run test_sms_contact as I don't have an active SIM in my device. Every other contacts test is passing for me though.
Attachment #8520641 - Flags: review?(florin.strugariu)
(Assignee)

Updated

3 years ago
Attachment #8516638 - Attachment description: Camera → Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26030/files
Attachment #8516638 - Attachment is obsolete: false
(Assignee)

Updated

3 years ago
Attachment #8516638 - Attachment description: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26030/files → Camera
(Assignee)

Updated

3 years ago
Attachment #8516683 - Attachment is obsolete: false
(Assignee)

Updated

3 years ago
Attachment #8516975 - Attachment is obsolete: false
(Assignee)

Comment 10

3 years ago
Patches that have been merged are not obsolete.
Comment on attachment 8520641 [details]
Contacts

treeherder tests are failing
Attachment #8520641 - Flags: review?(florin.strugariu) → review-
(Assignee)

Comment 12

3 years ago
Comment on attachment 8520641 [details]
Contacts

The single failure is a known intermittent, and it didn't reproduce on a retrigger.
Attachment #8520641 - Flags: review- → review?(florin.strugariu)
Attachment #8520641 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8520641 [details]
Contacts

Merged in https://github.com/mozilla-b2g/gaia/commit/c98c9c68fa914b379a1b1beabc63389bece5c9c1
(Assignee)

Comment 14

3 years ago
Created attachment 8524415 [details] [review]
Cost control
Attachment #8524415 - Flags: review?(florin.strugariu)
(Assignee)

Comment 15

3 years ago
Created attachment 8524479 [details] [review]
Email

Note that this does not resolve bug 1090281, which I have seen locally whilst testing.
Attachment #8524479 - Flags: review?(florin.strugariu)
(Assignee)

Comment 16

3 years ago
Created attachment 8524485 [details] [review]
Emergency call
Attachment #8524485 - Flags: review?(florin.strugariu)
(Assignee)

Comment 17

3 years ago
Created attachment 8524510 [details] [review]
FM radio
Attachment #8524510 - Flags: review?(florin.strugariu)
(Assignee)

Comment 18

3 years ago
Created attachment 8524513 [details] [review]
First time use
Attachment #8524513 - Flags: review?(florin.strugariu)
Attachment #8524485 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8524485 [details] [review]
Emergency call

https://github.com/mozilla-b2g/gaia/commit/5fd781a5d3f80b94dce8d17a1ab5b3a3601045ff
Attachment #8524510 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8524510 [details] [review]
FM radio

https://github.com/mozilla-b2g/gaia/commit/ae8083210d21edffdbd014c86cc06b71d08c9c3a
Attachment #8524415 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8524415 [details] [review]
Cost control

https://github.com/mozilla-b2g/gaia/commit/e9b1a8c222bf674cbf960015832a306bacfe3000
Attachment #8524513 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8524513 [details] [review]
First time use

https://github.com/mozilla-b2g/gaia/commit/b32e14bc9bc0483ee26b785d75f08f7d1c8ff59a
Attachment #8524479 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8524479 [details] [review]
Email

merged https://github.com/mozilla-b2g/gaia/commit/0da467aba9ecf354b905f9cc08dfa1a4169659e4
(Assignee)

Comment 24

3 years ago
Created attachment 8528329 [details] [review]
Gallery
Attachment #8528329 - Flags: review?(florin.strugariu)
Attachment #8528329 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8528329 [details] [review]
Gallery

merged in https://github.com/mozilla-b2g/gaia/commit/625239c492bc7c7e00db815e15448fbfe87f983c
(Assignee)

Comment 26

3 years ago
Created attachment 8529193 [details] [review]
Homescreen

Please note that I have been unable to run the tests that install an app due an issue reaching my local web server from the device. All other homescreen tests are passing for me.
Attachment #8529193 - Flags: review?(florin.strugariu)
Attachment #8529193 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8529193 [details] [review]
Homescreen

https://github.com/mozilla-b2g/gaia/commit/609f3985df16ac2505a48a699ae9406b978ac03c
(Assignee)

Comment 28

3 years ago
Created attachment 8530884 [details] [review]
Keyboard
Attachment #8530884 - Flags: review?(florin.strugariu)
(Assignee)

Comment 29

3 years ago
Created attachment 8530893 [details] [review]
Lockscreen
Attachment #8530893 - Flags: review?(florin.strugariu)
(Assignee)

Comment 30

3 years ago
Created attachment 8530894 [details] [review]
Messages

I've not been able to run the tests as I'm traveling and they require a carrier. I have triggered an adhoc build in Jenkins though: http://jenkins1.qa.scl3.mozilla.com:8080/view/UI/job/flame-kk.ui.adhoc/371/
Attachment #8530894 - Flags: review?(florin.strugariu)
Comment hidden (obsolete)
Comment hidden (obsolete)
Attachment #8530884 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8530884 [details] [review]
Keyboard

https://github.com/mozilla-b2g/gaia/commit/d5b610ecce6ccd791d4275ff730f6af717d08b9b
(Assignee)

Comment 34

3 years ago
(In reply to Dave Hunt (:davehunt) from comment #30)
> Created attachment 8530894 [details] [review]
> Messages
> 
> I've not been able to run the tests as I'm traveling and they require a
> carrier. I have triggered an adhoc build in Jenkins though:
> http://jenkins1.qa.scl3.mozilla.com:8080/view/UI/job/flame-kk.ui.adhoc/371/

I've fixed an issue, rebased, and pushed. New adhoc: http://jenkins1.qa.scl3.mozilla.com:8080/view/UI/job/flame-kk.ui.adhoc/377/
Comment on attachment 8530893 [details] [review]
Lockscreen

LGTM
Attachment #8530893 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8530893 [details] [review]
Lockscreen

https://github.com/mozilla-b2g/gaia/commit/1dee279381fbc188fa1f083d8f17c20f8c3af07d
Comment on attachment 8530894 [details] [review]
Messages

LGTM

Tested this locally and it works OK
Attachment #8530894 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8530894 [details] [review]
Messages

merged in https://github.com/mozilla-b2g/gaia/commit/12606843268c9c0478d67388cd6fb6f86eb6d923
(Assignee)

Comment 39

3 years ago
Created attachment 8534934 [details] [review]
Music
Attachment #8534934 - Flags: review?(florin.strugariu)
(Assignee)

Comment 40

3 years ago
Created attachment 8534936 [details] [review]
Persona
Attachment #8534936 - Flags: review?(florin.strugariu)
Attachment #8534934 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8534934 [details] [review]
Music

merged in https://github.com/mozilla-b2g/gaia/commit/b0e5a07bd0d96764fdce566ca13cb4395c69bbaf
Attachment #8534936 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8534936 [details] [review]
Persona

merged in: https://github.com/mozilla-b2g/gaia/commit/35f2f7dbecd05f1ee0ffca3994a894a1c90ad2ef
(Assignee)

Comment 43

3 years ago
Created attachment 8535243 [details] [review]
Phone

The DSDS tests were failing for me locally, but then I only have one SIM.
Attachment #8535243 - Flags: review?(florin.strugariu)
Comment on attachment 8535243 [details] [review]
Phone

some dialer tests are failing 
see: http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/436/console
Attachment #8535243 - Flags: review?(florin.strugariu) → review-
I guess this has to be:
https://github.com/mozilla-b2g/gaia/pull/26724/files#diff-193d67b00241c0a135318265c7b61febR102
+ Wait(self.marionette).until(call.is_displayed() and contact.text != u'')
(In reply to Dave Hunt (:davehunt) from comment #39)
> Created attachment 8534934 [details] [review]
> Music

While testing one of the music testcase, I fail on this line:
 Wait(self.marionette).until(expected.element_not_displayed(
+ *self._loading_spinner_locator))

I think element_not_displayed takes an element as an argument, not a locator, right? I'm surprised other people don't get the same failure as me for this.
Also a problem in the wait_for_music_tiles_displayed function.
(Assignee)

Comment 48

3 years ago
(In reply to Martijn Wargers [:mwargers] (QA) from comment #46)
> (In reply to Dave Hunt (:davehunt) from comment #39)
> > Created attachment 8534934 [details] [review]
> > Music
> 
> While testing one of the music testcase, I fail on this line:
>  Wait(self.marionette).until(expected.element_not_displayed(
> + *self._loading_spinner_locator))
> 
> I think element_not_displayed takes an element as an argument, not a
> locator, right? I'm surprised other people don't get the same failure as me
> for this.

Please update to the latest Marionette client. The element visibility checks were updated in bug  	1015242 to accept locators.
Created attachment 8544849 [details] [review]
System

I did the rewrite of the system app. Locally, the tests in system/ are working with these changes.
Attachment #8544849 - Flags: review?(dave.hunt)
(Assignee)

Comment 50

3 years ago
Comment on attachment 8544849 [details] [review]
System

Thanks Martijn. A few of these waits can be improved now that we can combine present/visible checking. Also, when checking that an element is not displayed after an action we should ideally first assert that it is displayed before that action.
Attachment #8544849 - Attachment description: systemapp → System
Attachment #8544849 - Flags: review?(dave.hunt) → review-
(Assignee)

Comment 51

3 years ago
Comment on attachment 8535243 [details] [review]
Phone

I believe I've fixed the issues. New adhoc run: http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/559
Attachment #8535243 - Flags: review- → review?(florin.strugariu)
(Assignee)

Comment 52

3 years ago
Created attachment 8545322 [details] [review]
Search

Adhoc: http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/561/
Attachment #8545322 - Flags: review?(florin.strugariu)
(Assignee)

Comment 53

3 years ago
(In reply to Dave Hunt (:davehunt) from comment #51)
> Comment on attachment 8535243 [details] [review]
> Phone
> 
> I believe I've fixed the issues. New adhoc run:
> http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/559

There are a couple of failures, but I also see these in the other Jenkins jobs.
(Assignee)

Comment 54

3 years ago
(In reply to Dave Hunt (:davehunt) from comment #52)
> Adhoc: http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/561/

The failures appear to be unrelated.
Attachment #8545322 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8545322 [details] [review]
Search

Merged in https://github.com/mozilla-b2g/gaia/commit/165833be6075194e9d75dc6da97d5ed16b529878
Comment on attachment 8544849 [details] [review]
System

Ok, I think I addressed all of your review comments.
Attachment #8544849 - Flags: review- → review?(dave.hunt)
(Assignee)

Comment 57

3 years ago
Comment on attachment 8544849 [details] [review]
System

A few minor fixes left, and I found a use of wait_for_element_not_displayed in UtilityTray.
Attachment #8544849 - Flags: review?(dave.hunt) → review-
Comment on attachment 8535243 [details] [review]
Phone

Looks ok to me
The fails are not related with the pull
Attachment #8535243 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8535243 [details] [review]
Phone

Dave can you update the pull it has merge conflicts 
Thanks!
Flags: needinfo?(dave.hunt)
(Assignee)

Comment 60

3 years ago
(In reply to Florin Strugariu [:Bebe] from comment #59)
> Comment on attachment 8535243 [details] [review]
> Phone
> 
> Dave can you update the pull it has merge conflicts 
> Thanks!

Updated but due to the changes probably worth another review and adhoc run.
Flags: needinfo?(dave.hunt)
(Assignee)

Updated

3 years ago
Attachment #8535243 - Flags: review+ → review?(florin.strugariu)
(In reply to Dave Hunt (:davehunt) from comment #57)
> Comment on attachment 8544849 [details] [review]
> System
> 
> A few minor fixes left, and I found a use of wait_for_element_not_displayed
> in UtilityTray.

Thanks for the review, just updated the PR with your review comments, sorry for the delay.

Updated

3 years ago
Attachment #8544849 - Flags: review- → review?(dave.hunt)
(Assignee)

Updated

3 years ago
Attachment #8544849 - Flags: review?(dave.hunt) → review-
Comment on attachment 8544849 [details] [review]
System

Updated PR again.
Attachment #8544849 - Flags: review- → review?(dave.hunt)
(Assignee)

Comment 63

3 years ago
Comment on attachment 8544849 [details] [review]
System

Still a couple of outstanding issues. Also, it's easier for the reviewer if you do not rebase/squash while a pull request is still in review. If you push additional commits it's much clearer to see what has been addressed from previous reviews. Rebasing/squashing can then happen after the pull has passed review.
Attachment #8544849 - Flags: review?(dave.hunt) → review-
Comment on attachment 8544849 [details] [review]
System

Ok, like this then?
Attachment #8544849 - Flags: review- → review?(dave.hunt)
(Assignee)

Comment 65

3 years ago
Comment on attachment 8544849 [details] [review]
System

Failures in gaia try: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=6e847c8624bb

Caused by a missed import of expected, and passing an element to element_not_present when a locator is needed.

I've also triggered a full adhoc:
http://jenkins1.qa.scl3.mozilla.com/view/UI/job/flame-kk.ui.adhoc/604/
Attachment #8544849 - Flags: review?(dave.hunt) → review-
Comment on attachment 8535243 [details] [review]
Phone

Tested this locally and works as expected.
Attachment #8535243 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8535243 [details] [review]
Phone

https://github.com/mozilla-b2g/gaia/commit/3060d16e5abd0f5a01a2b72c350be6aceb5fba46
Created attachment 8553106 [details] [review]
systemapp2

Sorry, I updated my master branch and updated the pull request with that updated branch. This should have fixed the failures in comment 65:
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=d51315b1f5c0
Attachment #8544849 - Attachment is obsolete: true
Comment on attachment 8553106 [details] [review]
systemapp2

Treeherder try is green with this.
Attachment #8553106 - Flags: review?(dave.hunt)
(Assignee)

Comment 70

3 years ago
Comment on attachment 8553106 [details] [review]
systemapp2

Unfortunately the imports changed while this patch has been in progress. We need to make sure they're in a try/except block so that we can gracefully switch to the marionette driver package.
Attachment #8553106 - Flags: review?(dave.hunt) → review-
(Assignee)

Comment 71

3 years ago
Created attachment 8555808 [details] [review]
Settings
Attachment #8555808 - Flags: review?(florin.strugariu)
Comment on attachment 8555808 [details] [review]
Settings

all looks OK
Attachment #8555808 - Flags: review?(florin.strugariu) → review+
(Assignee)

Comment 73

3 years ago
(In reply to Florin Strugariu [:Bebe] from comment #72)
> Comment on attachment 8555808 [details] [review]

Landed in:
https://github.com/mozilla-b2g/gaia/commit/a0529167d625c8a99d58ff249b12b98bd2aff973

Comment 74

3 years ago
Created attachment 8558644 [details] [review]
System

Updated

3 years ago
Attachment #8558644 - Flags: review?(dave.hunt)
(Assignee)

Comment 75

3 years ago
Comment on attachment 8558644 [details] [review]
System

Looks good, thanks. Landed in:
https://github.com/mozilla-b2g/gaia/commit/3912b4b5594688758c248f825020ed184df1e69f
Attachment #8558644 - Attachment description: [PullReq] mwargers:systemapp3 to mozilla-b2g:master → System
Attachment #8558644 - Flags: review?(dave.hunt) → review+
(Assignee)

Updated

3 years ago
Attachment #8553106 - Attachment is obsolete: true
Dave, what needs to be still done for this bug to get resolved?
Flags: needinfo?(dave.hunt)
(Assignee)

Comment 77

3 years ago
The follow app objects still need to be updated:
* testapp
* ui_tests
* ui_tests_privileged
* videoplayer
* wallpaper

Following that, we should remove any remaining uses of the wait_for methods in the tests and base classes. Something that might speed this up is the removal of the endurance tests - I believe they're no longer being run/maintained.

This is not a high priority task. I'm happy for it to remain assigned to me and I'll submit patches as and when I can. I typically work on these patches while commuting to the London office, and I've not been in for a few weeks.
Flags: needinfo?(dave.hunt)
(In reply to Dave Hunt (:davehunt) from comment #77)
> Something that might speed this up is the
> removal of the endurance tests - I believe they're no longer being
> run/maintained.

That's bug 1117144. Thanks for the info.

Comment 79

3 years ago
Created attachment 8579311 [details] [review]
Test container
(Assignee)

Updated

3 years ago
Attachment #8579311 - Attachment description: [gaia] davehunt:bug1074117-testapp > mozilla-b2g:master → Test Container
(Assignee)

Updated

3 years ago
Attachment #8579311 - Attachment description: Test Container → Test container
(Assignee)

Updated

3 years ago
Attachment #8579311 - Attachment description: Test container → Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/28950
Attachment #8579311 - Flags: review?(martijn.martijn)

Comment 80

3 years ago
Created attachment 8579314 [details] [review]
UI tests
(Assignee)

Updated

3 years ago
Attachment #8579314 - Attachment description: [gaia] davehunt:bug1074117-uitests > mozilla-b2g:master → UI tests
Attachment #8579314 - Flags: review?(martijn.martijn)
(Assignee)

Updated

3 years ago
Attachment #8579311 - Attachment description: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/28950 → Test container
Comment on attachment 8579311 [details] [review]
Test container

The changes looke fine to me.
Also tested test_system_message.py locally on device and desktop and it still passes with these changes.
Attachment #8579311 - Flags: review?(martijn.martijn) → review+
Comment on attachment 8579314 [details] [review]
UI tests

The changes look fine to me. There is 1 obvious mistake you have in the patch which caused some local failures. Other than that, it seems fine to me.
I tested on device: test_privileged_app_geolocation_prompt.py, keyboard tests, test_a11y_keyboard_key_names.py and system tests.
Attachment #8579314 - Flags: review?(martijn.martijn) → review-
(Assignee)

Comment 83

3 years ago
Comment on attachment 8579314 [details] [review]
UI tests

Fixed the copy/paste issue.
Attachment #8579314 - Flags: review- → review?(martijn.martijn)

Updated

3 years ago
Attachment #8579314 - Flags: review?(martijn.martijn) → review+
(Assignee)

Comment 84

3 years ago
(In reply to Martijn Wargers [:mwargers] (QA) from comment #81)
> Comment on attachment 8579311 [details] [review]
> Test container
> 
> The changes looke fine to me.
> Also tested test_system_message.py locally on device and desktop and it
> still passes with these changes.

Landed in:
https://github.com/mozilla-b2g/gaia/commit/984af3a2eeb978d5125454196ea7b0d9074fcf1a
(Assignee)

Comment 85

3 years ago
(In reply to Dave Hunt (:davehunt) from comment #83)
> Comment on attachment 8579314 [details] [review]
> UI tests
> 
> Fixed the copy/paste issue.

Landed in:
https://github.com/mozilla-b2g/gaia/commit/0409755bd0e93d229bd43f8d5378e6dbc5e2f74c

Comment 86

3 years ago
Created attachment 8584492 [details] [review]
Video player
(Assignee)

Updated

3 years ago
Attachment #8584492 - Attachment description: [gaia] davehunt:bug1074117-videoplayer > mozilla-b2g:master → Video player
Attachment #8584492 - Flags: review?(martijn.martijn)

Comment 87

3 years ago
Created attachment 8584493 [details] [review]
Wallpaper
(Assignee)

Updated

3 years ago
Attachment #8584493 - Attachment description: [gaia] davehunt:bug1074117-wallpaper > mozilla-b2g:master → Wallpaper
Attachment #8584493 - Flags: review?(martijn.martijn)
Comment on attachment 8584492 [details] [review]
Video player

Looks fine and the relevant tests work locally on b2g desktop for me.
Attachment #8584492 - Flags: review?(martijn.martijn) → review+
Comment on attachment 8584493 [details] [review]
Wallpaper

It looks fine, just some question I have in the pull request. If you can answer that, then it's an r+.
The test_settings_wallpaper.py test that is affected by it, works fine on b2g desktop, anyway.
Attachment #8584493 - Flags: review?(martijn.martijn) → review+
(Assignee)

Comment 90

3 years ago
(In reply to Martijn Wargers [:mwargers] (QA) from comment #88)
> Comment on attachment 8584492 [details] [review]
> Video player
> 
> Looks fine and the relevant tests work locally on b2g desktop for me.

Landed in:
https://github.com/mozilla-b2g/gaia/commit/71c3b0860eea690a088860192b578fc2aab0c497
(Assignee)

Comment 91

3 years ago
(In reply to Martijn Wargers [:mwargers] (QA) from comment #89)
> Comment on attachment 8584493 [details] [review]
> Wallpaper
> 
> It looks fine, just some question I have in the pull request. If you can
> answer that, then it's an r+.
> The test_settings_wallpaper.py test that is affected by it, works fine on
> b2g desktop, anyway.

Landed in:
https://github.com/mozilla-b2g/gaia/commit/8da1599f7742ad3f90ad324cd31ce2f7dbca4025
(Assignee)

Comment 92

2 years ago
I'm going to mark this as resolved as we covered most of the explicit waits.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.