Closed Bug 1145112 Opened 5 years ago Closed 5 years ago

Convert all usages of `wait_for_condition` to `Wait().until()`

Categories

(Testing :: Firefox UI Tests, defect)

defect
Not set

Tracking

(firefox41 fixed, firefox-esr38 fixed)

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed
firefox-esr38 --- fixed

People

(Reporter: whimboo, Assigned: sandeep, Mentored)

References

()

Details

(Whiteboard: [lang=py][good first bug])

Attachments

(2 files, 2 obsolete files)

52 bytes, text/x-github-pull-request
whimboo
: review+
whimboo
: review-
Details | Review
53 bytes, text/x-github-pull-request
whimboo
: review+
Details | Review
In our tests we are using a lot the not reliable `wait_for_condition` method. All those instances should be converted to use `Wait().until()`.

When you check the repository, a couple of tests make already use of it. So that can be taken as a template.
OK, I've cloned the Git repo to my machine and run `python setup.py develop` (what does that do exactly?).

The main subdirectories seem to be `firefox_ui_tests`, `firefox_ui_harness`, `firefox_puppeteer`.

So I have to convert all calls to `wait_for_condition()` in the tests in these subdirectories to `Wait().until()` and then run them using some tool?
Great to see that you got started on this bug! Yes, every instance of wait_for_condition has to be updated to use Wait().until(). Once you have changed all, create a pull request on github and wait what Travis CI tells you about test failures. This will take a couple of minutes. If all tests are passing, please create a text attachment for this bug, which only contains the link to the PR on github, and ask me for review or feedback. Depending on what you are comfortable with.

Let me know if you have other questions.
Assignee: nobody → sandeep
Status: NEW → ASSIGNED
Is there a relevant API that gives information about Wait().until()?  What are its arguments and return value/type?
Also, do I need to install any other testing tools to run tests once I make the code changes?  If there is a MDN documentation that describes this that would be helpful.
(In reply to Sandeep Murthy from comment #3)
> Is there a relevant API that gives information about Wait().until()?  What
> are its arguments and return value/type?
You can find this information here:
http://marionette-client.readthedocs.org/en/latest/reference.html?highlight=wait#wait
I've looked at the API and it seems Wait() accepts up to five parameters: marionette, timeout, interval, ignored_exceptions and clock.  In firefox_ui_tests/remote/security/test_submit_unencrypted_info_warning.py I found an example of Wait() being used, which is

    Wait(self.marionette, ignored_exceptions=NoAlertPresentException)

The timeout, interval and clock parameters are are not passed here, is this exactly how I should replace all other instances of wait_for_condition()?

Also, do I have to replicate the try-finally block each time I add the Wait() call?

I am assuming that I have to change all instances of wait_for_condition() to exactly this Wait() call type?
You do not have to replicate the usage of Wait as given above. The try/finally block is specific for this test. Also the optional parameters are not a must have, especially not ignored_exceptions. In case of timeout you only have to specify that if the wait_for_condition() call is also using a timeout other than 5s. That value is the default for until.
I've made and commited the changes to a topic branch (bugfix-1145112) of the repo cloned from the GitHub fork, and created the PR on GitHub.  Waiting for Travis CI tests to pass.

I have a small problem with the cloned repo - there are merge conflicts when I tried to rebase the topic branch against an updated version of the local master (I did a git fetch of the remote origin to the local master, then did a git rebase from the topic branch against the local master).

How would you advise to solve this?
Here is the conflict message:

git rebase master
First, rewinding head to replay your work on top of it...
Applying: Replaced all instances of wait_for_condition() with Wait().until()
Using index info to reconstruct a base tree...
M	firefox_ui_tests/remote/security/test_security_notification.py
Falling back to patching base and 3-way merge...
Auto-merging firefox_ui_tests/remote/security/test_security_notification.py
CONFLICT (content): Merge conflict in firefox_ui_tests/remote/security/test_security_notification.py
Failed to merge in the changes.
Patch failed at 0001 Replaced all instances of wait_for_condition() with Wait().until()
The copy of the patch that failed is found in:
   /Users/srm/Documents/sandeep/cst/mozilla/firefox-ui-tests/.git/rebase-apply/patch

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".
Hi

I need help with this conflict merge - I cannot see what I'm doing wrong with respect to Git, and the required code changes have been made.
Flags: needinfo?(hskupin)
The above merge conflict should be easy to fix. Just open the test test_security_notification.py and check for <<< and >>> lines. Maybe best would be if you replace the current content of this file with the one from upstream master, and re-enter your changes afterward. Once done do a "git add %testfile%", and continue with "git rebase --continue".

Btw. the PR we are talking about here is https://github.com/mozilla/firefox-ui-tests/pull/144.
Flags: needinfo?(hskupin)
I have the same problem as before, after following your steps (replaced local copy of firefox_ui_tests/remote/security/test_security_notification/py with upstream master copy on bug fix branch, then replaced wait_for_condition() with Wait().until(), then did a git add of this file, then did git rebase master from this branch).  I tried to fix the merge conflict in the same way, but have the same problem.

git rebase master
First, rewinding head to replay your work on top of it...
Applying: Replaced all instances of wait_for_condition() with Wait().until()
Using index info to reconstruct a base tree...
M	firefox_ui_tests/remote/security/test_security_notification.py
Falling back to patching base and 3-way merge...
Auto-merging firefox_ui_tests/remote/security/test_security_notification.py
CONFLICT (content): Merge conflict in firefox_ui_tests/remote/security/test_security_notification.py
Failed to merge in the changes.
Patch failed at 0001 Replaced all instances of wait_for_condition() with Wait().until()
The copy of the patch that failed is found in:
   /Users/srm/Documents/sandeep/cst/mozilla/firefox-ui-tests/.git/rebase-apply/patch

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".
(In reply to Sandeep Murthy from comment #12)
> I have the same problem as before, after following your steps (replaced
> local copy of firefox_ui_tests/remote/security/test_security_notification/py
> with upstream master copy on bug fix branch, then replaced
> wait_for_condition() with Wait().until(), then did a git add of this file,
> then did git rebase master from this branch).  I tried to fix the merge
> conflict in the same way, but have the same problem.

So here is the solution. Do all that AFTER you called `git rebase master`. This would apply to all the files which are in a CONFLICT situation. Only with a rebase you will get the right content in from master for a specific commit in the queue. Oh, sometimes it's easier to squash all commits into a single one before doing a rebase. So you are not forced to touch the same code for different conflicting commits.
OK I've fixed the merge conflict(s), and updated the GitHub pull request by pushing changes to the bugfix-1145112 branch of my fork.
Can you please add a text attachment with the PR URL in it, and request review? Similar to what I did on bug 1161737. Thanks.
Attached file firefox-ui-tests_GitHub_PR_144.txt (obsolete) —
Attachment #8602495 - Flags: review+
There are Travis CI build failures related to this (https://travis-ci.org/mozilla/firefox-ui-tests/builds/61535973), but appear to be as a result of some PEP-8 rules failing, not any code changes.
Comment on attachment 8602495 [details] [review]
firefox-ui-tests_GitHub_PR_144.txt

FYI you should not r+ an attachment but ask for review as I do now for myself. I will look at it later.
Attachment #8602495 - Flags: review+ → review?(hskupin)
Comment on attachment 8602495 [details] [review]
firefox-ui-tests_GitHub_PR_144.txt

Please see my comments on the PR of what needs to be fixed. In general this is going the right way. Thanks for the first patch!
Attachment #8602495 - Flags: review?(hskupin) → feedback+
I've resolved the PEP8 errors (and testing this locally using the `pep8` tool) but again rebasing the development branch against my updated local master causes problems.  Specifically, what I do is do a git fetch of the upstream master to the local master then merge, switch to the dev branch and do a git rebase -i master.  I choose only the commits I want, deleting those which are unnecessary, and squashing all the most recent relevant commits to the first one, but I get the following message:

git rebase -i master
error: could not apply 1a712f6... Replaced all instances of wait_for_condition() with Wait().until()

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".
Could not apply 1a712f6cb827e631d906138ac81d6fbfd10e0349... Replaced all instances of wait_for_condition() with Wait().until()
I've created a new pull request (https://github.com/mozilla/firefox-ui-tests/pull/197).

The Travis build has failed but this seems to be related to other issues, not the code changes I made.  For example all the PEP8 errors are resolved.
Flags: needinfo?(hskupin)
Thanks for the update Sandeep! I know that rebasing can be hard. I also hit it a couple of times. Merge issues like the above will occur more frequently if you have a longer list of commits on top of master's HEAD. In case you cannot cope with the merge issue, the best option would be to squash all commits on the dev branch to a single one - but by ONLY operating on the dev branch. That means you have to find the last official changeset and do a rebase like "git rebase -i %changeset%". A following rebase against master (as you did) will only bug you once with fixing merge issues.

For a review request please create a new attachment on this bug. Similar to the one which already exists (which you then can mark as obsolete). And request review from me when you think that you are ready for it. Thanks.
Flags: needinfo?(hskupin)
Comment on attachment 8602495 [details] [review]
firefox-ui-tests_GitHub_PR_144.txt

Superseded by new pull request https://github.com/mozilla/firefox-ui-tests/pull/197
Attachment #8602495 - Attachment is obsolete: true
Flags: needinfo?(hskupin)
Attachment #8602495 - Flags: review+
Attached file firefox-ui-tests_GitHub_PR_197.txt (obsolete) —
New pull request
Flags: needinfo?(hskupin)
Attachment #8624496 - Flags: review?(hskupin)
Comment on attachment 8624496 [details]
firefox-ui-tests_GitHub_PR_197.txt

https://github.com/mozilla/firefox-ui-tests/pull/197
Attachment #8624496 - Attachment description: New pull request → New pull requeset
Comment on attachment 8624496 [details]
firefox-ui-tests_GitHub_PR_197.txt

https://github.com/mozilla/firefox-ui-tests/pull/197
Attachment #8624496 - Attachment description: New pull requeset → https://github.com/mozilla/firefox-ui-tests/pull/197
Attachment #8624496 - Attachment mime type: text/plain → text/x-github-pull-request
Comment on attachment 8624496 [details]
firefox-ui-tests_GitHub_PR_197.txt

https://github.com/mozilla/firefox-ui-tests/pull/197
Attachment #8624496 - Attachment description: https://github.com/mozilla/firefox-ui-tests/pull/197 → firefox-ui-tests_GitHub_PR_197.txt
Sorry for these attempts to submit the attachment for the new pull request.  I'm not familiar with the Bugzilla interface.
You should ensure that your text editor saves the file as plain text. The attached file looks like a RTF format.
Attachment #8624496 - Attachment is obsolete: true
Attachment #8624496 - Flags: review?(hskupin)
Attachment #8624638 - Flags: review?(hskupin)
Comment on attachment 8624638 [details] [review]
github_pull_request.txt

Only some small things to be changed, then we are good to get the PR landed. Thanks!
Attachment #8624638 - Flags: review?(hskupin) → review-
Latest changes requested in the review have been made, please see the PR.  For some reason the last push I made from my local repo's dev branch `bugfix-1145112-new`, which created PR #197, made that branch disappear.  So I had to create clone my fork again and "create" the vanished branch using `git checkout -t origin/bugfix-1145112-new`.
Comment on attachment 8624638 [details] [review]
github_pull_request.txt

Thanks. FYI please do not miss to set the r? flag in the future. This update could get missed otherwise. I will have a look later today.
Attachment #8624638 - Flags: review?(hskupin)
Attachment #8624638 - Flags: review?(hskupin) → review-
Please see the updated PR.
Comment on attachment 8624638 [details] [review]
github_pull_request.txt

I squashed the commits and landed the PR as https://github.com/mozilla/firefox-ui-tests/commit/3b928dcc9e31879e7a79f449adb54439f5266549.

Please note that we will also have to update the unit tests of the firefox_puppeteer package. As best do a search for wait_for_condition across the whole folder to ensure you touch every instance. I will leave this bug open for the remaining work.

Thanks for the work so far!
Attachment #8624638 - Flags: review- → review+
This is still showing as assigned, I thought the pull request had resolved it.
Please read my last comment. As you know we also have to update all the tests under firefox_puppeteer which were not part of the first PR.
OK, no probs, a search for `wait_for_condition` in the `firefox_puppeteer` folder (on my Sublime Text 3) doesn't show any occurrences - the only one in `test_toolbars.py` was replaced and I've triggered another Travis build by closing and re-opening my pull request for puppeteer tests (#204).
The builds have passed.
As your PR shows there are a couple of wait_for_condition calls which had to be replaced. So if all is fine please make sure to add a new attachment with the github PR link included to this bug and ask for review. I will then have a look at it. Thanks!
Attachment #8626220 - Flags: review?(hskupin)
OK, I've attached the PR link in the text file.  Hope I have the right flags this time - "review?"
Comment on attachment 8626220 [details] [review]
Bug-1145112_GitHub_pull_request.txt

Great! Thanks a lot.
Attachment #8626220 - Flags: review?(hskupin) → review+
The second PR has been merged as:

https://github.com/mozilla/firefox-ui-tests/commit/f75e5cd76507357c97850799cbcdc679149168c1

So now every usage of wait_for_condition has been wiped out. This is wonderful. Thanks a lot Sandeep!
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
We would like to backport the patch from bug 1201715 to esr38. But given that this patch never landed on that branch, we get lots of merge conflicts. I will check what's necessary here to land it on esr38 too.
Bug 1170148 also landed some changes which were only appropriate for Firefox 41 and later. So two lines needed to be changed in the test_toolbar.py test. Otherwise easy merge selection.

Landed the backport as:
https://github.com/mozilla/firefox-ui-tests/compare/a8aaa07cdc3d...155a6fa2f71a
Is there anything I should do on this?
Thanks for asking, but there is all done on this bug. No more work is really necessary here. But if you want to help on some other bug please let me know.
Product: Mozilla QA → Testing
Hi Henrik,

There still are occurrences of "wait_for_condition" that can be replaced in the latest source, if that is what you want ?
Also, do let me know of the occurrences that I need not change and leave intact.

Thanks.
Flags: needinfo?(hskupin)
Aseem, I'm sorry! It looks like that I sent you the wrong link. The bug I was actually referring to is bug 1354458.
Flags: needinfo?(hskupin)
You need to log in before you can comment on or make changes to this bug.