1354458 - Get rid of `self.wait_for_condition()` in favor of `Wait().until()`

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The `self.wait_for_condition()` lacks various features and simply re-implements parts of what is present in `Wait().until()`.

I would suggest that we kill `self.wait_for_condition()` and make use of `Wait().until()` across all unit tests.

If we don't want to remove this method from the testcase class right now, we should at least mark it as deprecated.

Andreas, do you have any objections?

Comment 1

[Andreas Tolfsen ❲:ato❳]

I agree, that makes sense.

Comment 2

[Raajit Raj(:raajitr)]

Hi, can I work on this bug? I'm new to open source contribution so might need help regarding pushing the changes, Thanks.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Raajit, sure you can work on it. To get started please have a look at the user story. Everything is explained there. Please note that we usually don't assign bugs until the first patch has been uploaded.

In case you have questions please contact me here, or better via IRC. Thanks for your interest!

Comment 4

[Aseem Yadav]

Attachment: Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

Review commit: https://reviewboard.mozilla.org/r/131172/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131172/

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Raajit, looks like Aseem already uploaded a first version of the patch. Given a missing comment from him on this bug I just forgot that he wanted to work on it. So maybe you can find another one. Please don't hesitate to contact me on IRC.

Comment 6

[Aseem Yadav]

Hi Henrik,

I am awaiting the review comments on the uploaded patch.
Do let me know if any.

Thanks.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hm, I'm sure that I did the review. Not sure why it doesn't appear here. Let me check in mozreview.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

https://reviewboard.mozilla.org/r/131172/#review133820

Thank you for the patch Aseem. As you can see I added a lot of comments to the review. I hope that those are helpful for you to improve the patch.

Further you missed to also remove the `wait_for_condition()` method from Marionette itself. It's no longer needed. Doing so you might see which other tests need an update.

Let me know if you have questions.

::: testing/marionette/harness/marionette_harness/tests/unit/single_finger_functions.py:11
(Diff revision 1)
>  def wait_for_condition_else_raise(marionette, wait_for_condition, expected, script):
>      try:
> -        wait_for_condition(lambda m: expected in m.execute_script(script))
> +        Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> +            lambda m: expected in m.execute_script(script),
> +            message='The expected string has not been found',
> +        )

This is a generic method, and used in various other functions in this file, and even in test_single_finger_functions.py.

You will have to closely look what has to be changed, and if yes what to use. I would like to first let you check this out yourself before I help too much.

I would suggest that you remove the changes for this commit, and that you do a refactoring of it in separate commit on this bug.

::: testing/marionette/harness/marionette_harness/tests/unit/test_click.py:118
(Diff revision 1)
>          self.marionette.navigate(test_html)
>          self.marionette.find_element(By.ID, "overflowLink").click()
> -        self.wait_for_condition(lambda mn: self.marionette.title == "XHTML Test Page")
> +        Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> +            lambda mn: self.marionette.title == "XHTML Test Page",
> +            message='The test page has not been loaded',
> +        )

This will cause a conflict once my patch on bug 1335778 lands later today or tomorrow. With it the wait will be removed all together.

::: testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py:76
(Diff revision 1)
>      def test_confirm_accept(self):
>          self.marionette.find_element(By.ID, "tab-modal-confirm").click()
>          self.wait_for_alert()
>          alert = self.marionette.switch_to_alert()
>          alert.accept()
> -        self.wait_for_condition(
> +        Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(

The change here and all the others won't need the `timeout` argument, because the default value of `5s` will be enough. Please note that the calls to  `wait_for_condition()` don't specify this either.

::: testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py:78
(Diff revision 1)
>          self.wait_for_alert()
>          alert = self.marionette.switch_to_alert()
>          alert.accept()
> -        self.wait_for_condition(
> -            lambda mn: mn.find_element(By.ID, "confirm-result").text == "true")
> +        Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> +            lambda mn: mn.find_element(By.ID, "confirm-result").text == "true",
> +            message='The expected text for #confirm-result has not been found',

I would suggest to rephrase this message to something like: "Value of element 'confirm-result' is false". 

This would also apply to the other instances of `Wait().until().

::: testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py:189
(Diff revision 1)
>          self.marionette.find_element(By.ID, "onbeforeunload-handler").click()
> -        self.wait_for_condition(
> +        Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
>              lambda mn: mn.execute_script("""
>                return window.onbeforeunload !== null;
> -            """))
> +            """),
> +            message='The event onbeforeunload returned "null"',

The message should be: "Handler for 'onbeforeunload' has not been set".

::: testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py:204
(Diff revision 1)
>          self.marionette.find_element(By.ID, "onbeforeunload-handler").click()
> -        self.wait_for_condition(
> +        Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
>              lambda mn: mn.execute_script("""
>                return window.onbeforeunload !== null;
> -            """))
> +            """),
> +            message='The event onbeforeunload returned "null"',

Same as above.

::: testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py:214
(Diff revision 1)
>          self.assertTrue(alert.text.startswith("This page is asking you to confirm"))
>          alert.accept()
> -        self.wait_for_condition(lambda mn: mn.get_url() == "about:blank")
> +        Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> +            lambda mn: mn.get_url() == "about:blank",
> +            message='The expected blank page has not been loaded',
> +        )

Just rephrase it a bit: "The expected page 'about:blank' has not been loaded".

::: testing/marionette/harness/marionette_harness/tests/unit/test_mouse_action.py:58
(Diff revision 1)
>                  cm_el = self.marionette.find_element(By.ID, "contentAreaContextMenu")
>                  return cm_el.get_property("state")
>  
>          self.assertEqual("closed", context_menu_state())
>          self.action.context_click(click_el).perform()
>          self.wait_for_condition(lambda _: context_menu_state() == "open")

You missed to remove the old lines in this file.

::: testing/marionette/harness/marionette_harness/tests/unit/test_mouse_action.py:59
(Diff revision 1)
>                  return cm_el.get_property("state")
>  
>          self.assertEqual("closed", context_menu_state())
>          self.action.context_click(click_el).perform()
>          self.wait_for_condition(lambda _: context_menu_state() == "open")
> +        Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(

There is no need to specify this timeout. It's not triggering a page load but just opening the context menu.

::: testing/marionette/harness/marionette_harness/tests/unit/test_mouse_action.py:61
(Diff revision 1)
>          self.assertEqual("closed", context_menu_state())
>          self.action.context_click(click_el).perform()
>          self.wait_for_condition(lambda _: context_menu_state() == "open")
> +        Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> +            lambda _: context_menu_state() == "open",
> +            message='The expected context menu state "open" has not been found',

A better message here would be: "The context menu has not been opened".

::: testing/marionette/harness/marionette_harness/tests/unit/test_mouse_action.py:83
(Diff revision 1)
>          el = self.marionette.find_element(By.ID, "showbutton")
>          self.action.middle_click(el).perform()
>  
> -        self.wait_for_condition(lambda _: el.get_property("innerHTML") == "1")
> +        Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> +            lambda _: el.get_property("innerHTML") == "1",
> +            message='The expected "innerHTML" value has not been found',

Please rephrase: "The property 'innerHTML' does not equal '1'"

::: testing/marionette/harness/marionette_harness/tests/unit/test_mouse_action.py:97
(Diff revision 1)
>              go_button = self.marionette.find_element(By.ID, "urlbar-go-button")
>              self.action.click(go_button).perform()
>          self.wait_for_condition(lambda mn: mn.get_url() == data_uri)
> +        Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> +            lambda mn: mn.get_url() == data_uri,
> +            message='The expected URI has not been loaded',

Please use the same phrase as I gave above about:blank.

::: testing/marionette/harness/marionette_harness/tests/unit/test_mouse_action.py:128
(Diff revision 1)
>          self.assertEqual("closed", context_menu_state())
>          self.action.context_click(currtab).perform()
> -        self.wait_for_condition(lambda _: context_menu_state() == "open")
> +        Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> +            lambda _: context_menu_state() == "open",
> +            message='The expected context menu state "open" has not been found',
> +        )

Same as above.

::: testing/marionette/harness/marionette_harness/tests/unit/test_mouse_action.py:137
(Diff revision 1)
>  
>          self.wait_for_condition(lambda _: context_menu_state() == "closed")
> +        Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> +            lambda _: context_menu_state() == "closed",
> +            message='The expected context menu state "closed" has not been found',
> +        )

Same as above.

::: testing/marionette/harness/marionette_harness/tests/unit/test_set_window_size.py:38
(Diff revision 1)
>  
>          # valid size
>          width = self.max_width - 100
>          height = self.max_height - 100
>          self.marionette.set_window_size(width, height)
> -        self.wait_for_condition(lambda m: m.execute_script("return window.wrappedJSObject.rcvd_event;"))
> +        Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(

It's not causing a page load, so remove the timeout argument.

::: testing/marionette/harness/marionette_harness/tests/unit/test_set_window_size.py:40
(Diff revision 1)
>          width = self.max_width - 100
>          height = self.max_height - 100
>          self.marionette.set_window_size(width, height)
> -        self.wait_for_condition(lambda m: m.execute_script("return window.wrappedJSObject.rcvd_event;"))
> +        Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> +            lambda m: m.execute_script("return window.wrappedJSObject.rcvd_event;"),
> +            message='The expected wrappedJSObject has not been found',

This simply checks if the 'onresize' event has been caught. So please rephrase.

::: testing/marionette/harness/marionette_harness/tests/unit/test_set_window_size.py:83
(Diff revision 1)
>              window.wrappedJSObject.rcvd_event = true;
>          };
>          """)
>          self.marionette.maximize_window()
>          self.wait_for_condition(lambda m: m.execute_script("return window.wrappedJSObject.rcvd_event;"))
> +        Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(

Same as above.

Comment 9

[Aseem Yadav]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

https://reviewboard.mozilla.org/r/131172/#review133820

> This is a generic method, and used in various other functions in this file, and even in test_single_finger_functions.py.
> 
> You will have to closely look what has to be changed, and if yes what to use. I would like to first let you check this out yourself before I help too much.
> 
> I would suggest that you remove the changes for this commit, and that you do a refactoring of it in separate commit on this bug.

So, It would be better if I make another method where wait().until() has been specified as a replacement instead of the "wait_for_condition()", as I can see that "wait_for_condition_else_raise()" method raises an exception with some relevant info. printed with it when the exception occurs.
I am thinking of using assertRaises() wherever "wait_..._raise()" is called removing the call to wait_for_condition() altogether.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

https://reviewboard.mozilla.org/r/131172/#review133820

> So, It would be better if I make another method where wait().until() has been specified as a replacement instead of the "wait_for_condition()", as I can see that "wait_for_condition_else_raise()" method raises an exception with some relevant info. printed with it when the exception occurs.
> I am thinking of using assertRaises() wherever "wait_..._raise()" is called removing the call to wait_for_condition() altogether.

Before we continue on this file I would suggest that we first get all the other issues solved. With that you will get more insight and it might become easier to make the changes to this file.

Comment 11

[Aseem Yadav]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

https://reviewboard.mozilla.org/r/131172/#review133820

> Before we continue on this file I would suggest that we first get all the other issues solved. With that you will get more insight and it might become easier to make the changes to this file.

well !! The changes to the remaining issues has already been made. However, I am just not getting the hint you are trying to give me when you say that I would get an insight after making changes. I think that Wait().until() can serve an alternative to it as well, when provided with "timeout" value and the message portion printing the adequate information of the exception (if possible) that might occur.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

https://reviewboard.mozilla.org/r/131172/#review133820

> well !! The changes to the remaining issues has already been made. However, I am just not getting the hint you are trying to give me when you say that I would get an insight after making changes. I think that Wait().until() can serve an alternative to it as well, when provided with "timeout" value and the message portion printing the adequate information of the exception (if possible) that might occur.

If those changes have already been done, can you please push this update to mozreview? So I can do a sign-off on this commit? 

Regarding the other parts of your last reply... we are going to remove `wait_for_condition` completely. So `Wait().until()` will be the only way of waiting for a condition.

Comment 13

[Aseem Yadav]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

https://reviewboard.mozilla.org/r/131172/#review133820

> If those changes have already been done, can you please push this update to mozreview? So I can do a sign-off on this commit? 
> 
> Regarding the other parts of your last reply... we are going to remove `wait_for_condition` completely. So `Wait().until()` will be the only way of waiting for a condition.

The confusion I had earlier, was with method wait_for_condition_else_raise():
```
def wait_for_condition_else_raise(marionette, wait_for_condition, expected, script):
    try:
        wait_for_condition(lambda m: expected in m.execute_script(script))
    except TimeoutException as e:
        raise TimeoutException("{0} got {1} instead of {2}".format(
            e.message, marionette.execute_script(script), expected))
```
It is used in one file "single_finger_functions.py" and by the name it is clear that only purpose it serves apart from waiting, is to raise the exception and print the info.
After testing with Wait().until() I can see the same info. can be printed in the "message=" part.

I am pushing the updates, let me know if I have missed out on anything.

Comment 14

[Aseem Yadav]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131172/diff/1-2/

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

https://reviewboard.mozilla.org/r/131172/#review139580

The update looks good. I was able to close a lot of the open issues. While going through the changes I found a couple of other nits which you might want to address. Otherwise there is still a bit of work to do for the `wait_for_condition` helper in the single click shared module.

::: testing/marionette/harness/marionette_harness/tests/unit/single_finger_functions.py:8
(Diff revision 2)
>  from marionette_driver.by import By
> +from marionette_driver.wait import Wait
>  
>  
>  def wait_for_condition_else_raise(marionette, wait_for_condition, expected, script):
> -    try:
> +    Wait(marionette, timeout=30).until(

So the `wait_for_condition` call doesn't actually call the method from the MarionetteTestCase class, but the method as specified by the passed-in argument. So I would suggest that we get rid of this argument, and update all tests which make use of this method.

Also please note while the original timeout was 30s this would only apply for page load activities. By default we should use 5s, and so the timeout paramter can be removed from `Wait()`. If there would be a test which uses this method for a page load, then we would have to add a new function parameter for the timeout.

::: testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py:78
(Diff revision 2)
>      def test_confirm_accept(self):
>          self.marionette.find_element(By.ID, "tab-modal-confirm").click()
>          self.wait_for_alert()
>          alert = self.marionette.switch_to_alert()
>          alert.accept()
> -        self.wait_for_condition(
> +        Wait(self.marionette,).until(

Please drop the comma, which is not needed. This applies to all instances of `Wait()`.

::: testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py:80
(Diff revision 2)
>          self.wait_for_alert()
>          alert = self.marionette.switch_to_alert()
>          alert.accept()
> -        self.wait_for_condition(
> -            lambda mn: mn.find_element(By.ID, "confirm-result").text == "true")
> +        Wait(self.marionette,).until(
> +            lambda mn: mn.find_element(By.ID, "confirm-result").text == "true",
> +            message="Value of element 'confirm-result' is false")

In this case it will be `false` because it's a string. Please check the other changes in this file too.

::: testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py:88
(Diff revision 2)
>          self.marionette.find_element(By.ID, "tab-modal-confirm").click()
>          self.wait_for_alert()
>          alert = self.marionette.switch_to_alert()
>          alert.dismiss()
> -        self.wait_for_condition(
> -            lambda mn: mn.find_element(By.ID, "confirm-result").text == "false")
> +        Wait(self.marionette,).until(
> +            lambda mn: mn.find_element(By.ID, "confirm-result").text == "true",

You exchanged the expected value here. Please revert that.

::: testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py:168
(Diff revision 2)
>          alert = self.marionette.switch_to_alert()
>          alert.send_keys("Some text!")
>          alert.accept()
> -        self.wait_for_condition(
> -            lambda mn: mn.find_element(By.ID, "prompt-result").text == "Some text!")
> +        Wait(self.marionette,).until(
> +            lambda mn: mn.find_element(By.ID, "prompt-result").text == "Some text!",
> +            message="expected value of element 'prompt-result' is not found")

You missed to adjust the wording so it's identical with all the other messages.

::: testing/marionette/harness/marionette_harness/tests/unit/test_mouse_action.py:94
(Diff revision 2)
>              urlbar.send_keys(data_uri)
>              go_button = self.marionette.find_element(By.ID, "urlbar-go-button")
>              self.action.click(go_button).perform()
> -        self.wait_for_condition(lambda mn: mn.get_url() == data_uri)
> +        Wait(self.marionette,).until(
> +            lambda mn: mn.get_url() == data_uri,
> +            message="The expected 'data:text/html,<html></html>' page has not been loaded",

I wouldn't hard-code the data URL, but simply say "The expected data URI has not been loaded".

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

https://reviewboard.mozilla.org/r/131172/#review133820

> The confusion I had earlier, was with method wait_for_condition_else_raise():
> ```
> def wait_for_condition_else_raise(marionette, wait_for_condition, expected, script):
>     try:
>         wait_for_condition(lambda m: expected in m.execute_script(script))
>     except TimeoutException as e:
>         raise TimeoutException("{0} got {1} instead of {2}".format(
>             e.message, marionette.execute_script(script), expected))
> ```
> It is used in one file "single_finger_functions.py" and by the name it is clear that only purpose it serves apart from waiting, is to raise the exception and print the info.
> After testing with Wait().until() I can see the same info. can be printed in the "message=" part.
> 
> I am pushing the updates, let me know if I have missed out on anything.

I will mark this issue as fixed. Follow-up work as necessary is handled in the other newly filed issue.

Comment 17

[Aseem Yadav]

Okay, So I have made the changes according to the review and also, I think previously I forgot to import the Wait module to one of the files so I added that declaration too.
Should I upload the patch using the Bug ID as I can see its not marked as fixed, or refrain from doing so until as you mentioned some other issue is filed ?

Thanks,
Aseem

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Please simply push your latest changes to Mozreview so we can sign off on them. Make sure to check all the open issues there, and that you fixed them, or if not ask if its unclear. Thanks.

Comment 19

[Aseem Yadav]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131172/diff/2-3/

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

https://reviewboard.mozilla.org/r/131172/#review145066

Sorry that my review is coming in that late, but I was mostly out all last week and haven't had the time to look at it. Please see my inline comments for necessary updates, and questions.

::: testing/marionette/harness/marionette_harness/tests/unit/single_finger_functions.py:6
(Diff revisions 2 - 3)
>  from marionette_driver.errors import TimeoutException
>  from marionette_driver.by import By
>  from marionette_driver.wait import Wait
>  
> -
>  def wait_for_condition_else_raise(marionette, wait_for_condition, expected, script):

Please revert this line removal. The linter will fail on this one because 2 empy lines are expected.

::: testing/marionette/harness/marionette_harness/tests/unit/single_finger_functions.py:12
(Diff revisions 2 - 3)
>      Wait(marionette, timeout=30).until(
>              lambda m: expected in m.execute_script(script),
>              message="got {0} instead of {1}".format(marionette.execute_script(script), expected),
>          )
>  
>  def press_release(marionette, times, wait_for_condition, expected):

You are removing the `wait_for_condition` parameter from the calls to any of the methods in this file inside the test file. So you will also have to do it here.

Please remember to run the tests locally before submitting an updated version of the patch to mozreview.

::: testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py:11
(Diff revisions 2 - 3)
>  from marionette_driver.expected import element_present
>  from marionette_driver import errors
>  from marionette_driver.marionette import Alert
>  from marionette_driver.wait import Wait
>  
> -from marionette_harness import MarionetteTestCase, skip_if_e10s, WindowManagerMixin
> +from marionette_harness import MarionetteTestCase, WindowManagerMixin

Please update to the latest-mozilla central code, where it has already been removed.

::: testing/marionette/harness/marionette_harness/tests/unit/test_set_window_size.py:38
(Diff revision 2)
>  
>          # valid size
>          width = self.max_width - 100
>          height = self.max_height - 100
>          self.marionette.set_window_size(width, height)
> -        self.wait_for_condition(lambda m: m.execute_script("return window.wrappedJSObject.rcvd_event;"))
> +        Wait(self.marionette,).until(

Why are you reverting to use `self.wait_for_condition` here?

::: testing/marionette/harness/marionette_harness/tests/unit/test_set_window_size.py:81
(Diff revision 2)
>          window.onresize = function() {
>              window.wrappedJSObject.rcvd_event = true;
>          };
>          """)
>          self.marionette.maximize_window()
>          self.wait_for_condition(lambda m: m.execute_script("return window.wrappedJSObject.rcvd_event;"))

Same here.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

https://reviewboard.mozilla.org/r/131172/#review139580

> You exchanged the expected value here. Please revert that.

I don't see that this has been fixed in the latest update.

> You missed to adjust the wording so it's identical with all the other messages.

Mind still using a capital letter for the first letter?

Comment 22

[Aseem Yadav]

Hi Henrik,

I came across an issue where mouse events aren't working as expected for testcase "test_single_finger_desktop.py". This file was using the methods where changes have been made. Below is the log:-

===================================================================

$ mach marionette-test testing\\marionette\\harness\\marionette_harness\\tests\\unit\\test_single_finger_desktop.py
 0:00.00 LOG: MainThread INFO Using workspace for temporary data: "c:\mozilla-source\mozilla-central"
 0:00.01 LOG: MainThread mozversion INFO application_buildid: 20170426184211
 0:00.01 LOG: MainThread mozversion INFO application_changeset: 0f5ba06c4c5959030a05cb852656d854065e2226
 0:00.01 LOG: MainThread mozversion INFO application_display_name: Nightly
 0:00.01 LOG: MainThread mozversion INFO application_id: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
 0:00.01 LOG: MainThread mozversion INFO application_name: Firefox
 0:00.01 LOG: MainThread mozversion INFO application_remotingname: firefox
 0:00.01 LOG: MainThread mozversion INFO application_vendor: Mozilla
 0:00.01 LOG: MainThread mozversion INFO application_version: 55.0a1
 0:00.01 LOG: MainThread mozversion INFO platform_buildid: 20170426184211
 0:00.02 LOG: MainThread mozversion INFO platform_changeset: 0f5ba06c4c5959030a05cb852656d854065e2226
 0:00.02 LOG: MainThread mozversion INFO platform_version: 55.0a1
 0:00.03 LOG: MainThread INFO Application command: c:/mozilla-source/mozilla-central/obj-x86_64-pc-mingw32\dist\bin\firefox.exe -no-remote -marionette -profile c:\users\compas~1\appdata\local\temp\tmpormkif.mozrunner
 0:08.27 LOG: MainThread INFO Profile path is c:\users\compas~1\appdata\local\temp\tmpormkif.mozrunner
 0:08.29 LOG: MainThread INFO Starting fixture servers
 0:12.51 LOG: MainThread INFO Fixture server listening on http://127.0.0.1:55867/
 0:12.51 LOG: MainThread INFO Fixture server listening on https://127.0.0.1:55872/
 0:12.57 LOG: MainThread INFO e10s is enabled
 0:12.57 LOG: MainThread mozversion INFO application_buildid: 20170426184211
 0:12.57 LOG: MainThread mozversion INFO application_changeset: 0f5ba06c4c5959030a05cb852656d854065e2226
 0:12.57 LOG: MainThread mozversion INFO application_display_name: Nightly
 0:12.57 LOG: MainThread mozversion INFO application_id: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
 0:12.57 LOG: MainThread mozversion INFO application_name: Firefox
 0:12.57 LOG: MainThread mozversion INFO application_remotingname: firefox
 0:12.57 LOG: MainThread mozversion INFO application_vendor: Mozilla
 0:12.58 LOG: MainThread mozversion INFO application_version: 55.0a1
 0:12.58 LOG: MainThread mozversion INFO platform_buildid: 20170426184211
 0:12.58 LOG: MainThread mozversion INFO platform_changeset: 0f5ba06c4c5959030a05cb852656d854065e2226
 0:12.58 LOG: MainThread mozversion INFO platform_version: 55.0a1
 0:12.58 SUITE_START: MainThread 1
 0:12.60 TEST_START: MainThread test_single_finger_desktop.py testSingleFingerMouse.test_chain
 0:47.84 TEST_END: MainThread ERROR, expected PASS
Traceback (most recent call last):
  File "c:\mozilla-source\mozilla-central\testing/marionette/harness\marionette_harness\marionette_test\testcases.py", line 166, in run
    testMethod()
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\test_single_finger_desktop.py", line 114, in test_chain
    chain(self.marionette, "button1-mousemove-mousedown", "delayed-mousemove-mouseup")
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\single_finger_functions.py", line 54, in chain
    wait_for_condition_else_raise(marionette, expected1, "return document.getElementById('button1').innerHTML;")
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\single_finger_functions.py", line 9, in wait_for_condition_else_raise
    message="got {0} instead of {1}".format(marionette.execute_script(script), expected),
  File "c:\mozilla-source\mozilla-central\testing/marionette/client\marionette_driver\wait.py", line 150, in until
    cause=last_exc)
TimeoutException: Timed out after 30.0 seconds with message: got button1-touchstart instead of button1-mousemove-mousedown
 0:47.86 TEST_START: MainThread test_single_finger_desktop.py testSingleFingerMouse.test_chain_flick
 1:18.27 TEST_END: MainThread ERROR, expected PASS
Traceback (most recent call last):
  File "c:\mozilla-source\mozilla-central\testing/marionette/harness\marionette_harness\marionette_test\testcases.py", line 166, in run
    testMethod()
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\test_single_finger_desktop.py", line 117, in test_chain_flick
    chain_flick(self.marionette, "button1-mousemove-mousedown-mousemove", "buttonFlick-mousemove-mouseup")
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\single_finger_functions.py", line 64, in chain_flick
    wait_for_condition_else_raise(marionette, expected1,"return document.getElementById('button1').innerHTML;")
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\single_finger_functions.py", line 9, in wait_for_condition_else_raise
    message="got {0} instead of {1}".format(marionette.execute_script(script), expected),
  File "c:\mozilla-source\mozilla-central\testing/marionette/client\marionette_driver\wait.py", line 150, in until
    cause=last_exc)
TimeoutException: Timed out after 30.0 seconds with message: got button1 instead of button1-mousemove-mousedown-mousemove
 1:18.29 TEST_START: MainThread test_single_finger_desktop.py testSingleFingerMouse.test_context_menu
 1:18.29 TEST_END: MainThread SKIP
 1:18.30 TEST_START: MainThread test_single_finger_desktop.py testSingleFingerMouse.test_double_tap
 1:48.67 TEST_END: MainThread ERROR, expected PASS
Traceback (most recent call last):
  File "c:\mozilla-source\mozilla-central\testing/marionette/harness\marionette_harness\marionette_test\testcases.py", line 166, in run
    testMethod()
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\test_single_finger_desktop.py", line 123, in test_double_tap
    double_tap(self.marionette, "button1-mousemove-mousedown-mouseup-click-mousemove-mousedown-mouseup-click")
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\single_finger_functions.py", line 130, in double_tap
    wait_for_condition_else_raise(marionette, expected, "return document.getElementById('button1').innerHTML;")
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\single_finger_functions.py", line 9, in wait_for_condition_else_raise
    message="got {0} instead of {1}".format(marionette.execute_script(script), expected),
  File "c:\mozilla-source\mozilla-central\testing/marionette/client\marionette_driver\wait.py", line 150, in until
    cause=last_exc)
TimeoutException: Timed out after 30.1 seconds with message: got button1-touchstart-touchend-mousemove-mousedown-mouseup-click-touchstart-touchend-mousemove-mousedown-mouseup-click instead of button1-mousemove-mousedown-mouseup-click-mousemove-mousedown-mouseup-click
 1:48.70 TEST_START: MainThread test_single_finger_desktop.py testSingleFingerMouse.test_long_press_action
 1:48.70 TEST_END: MainThread SKIP
 1:48.70 TEST_START: MainThread test_single_finger_desktop.py testSingleFingerMouse.test_long_press_fail
 1:48.70 TEST_END: MainThread SKIP
 1:48.70 TEST_START: MainThread test_single_finger_desktop.py testSingleFingerMouse.test_long_press_on_xy_action
 1:48.70 TEST_END: MainThread SKIP
 1:48.70 TEST_START: MainThread test_single_finger_desktop.py testSingleFingerMouse.test_move_by_offset
 2:19.00 TEST_END: MainThread ERROR, expected PASS
Traceback (most recent call last):
  File "c:\mozilla-source\mozilla-central\testing/marionette/harness\marionette_harness\marionette_test\testcases.py", line 166, in run
    testMethod()
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\test_single_finger_desktop.py", line 80, in test_move_by_offset
    move_element_offset(self.marionette, "button1-mousemove-mousedown", "button2-mousemove-mouseup")
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\single_finger_functions.py", line 43, in move_element_offset
    wait_for_condition_else_raise(marionette, expected1, "return document.getElementById('button1').innerHTML;")
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\single_finger_functions.py", line 9, in wait_for_condition_else_raise
    message="got {0} instead of {1}".format(marionette.execute_script(script), expected),
  File "c:\mozilla-source\mozilla-central\testing/marionette/client\marionette_driver\wait.py", line 150, in until
    cause=last_exc)
TimeoutException: Timed out after 30.0 seconds with message: got button1 instead of button1-mousemove-mousedown
 2:19.02 TEST_START: MainThread test_single_finger_desktop.py testSingleFingerMouse.test_move_element
 2:49.36 TEST_END: MainThread ERROR, expected PASS
Traceback (most recent call last):
  File "c:\mozilla-source\mozilla-central\testing/marionette/harness\marionette_harness\marionette_test\testcases.py", line 166, in run
    testMethod()
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\test_single_finger_desktop.py", line 77, in test_move_element
    move_element(self.marionette, "button1-mousemove-mousedown", "button2-mousemove-mouseup")
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\single_finger_functions.py", line 33, in move_element
    wait_for_condition_else_raise(marionette, expected1, "return document.getElementById('button1').innerHTML;")
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\single_finger_functions.py", line 9, in wait_for_condition_else_raise
    message="got {0} instead of {1}".format(marionette.execute_script(script), expected),
  File "c:\mozilla-source\mozilla-central\testing/marionette/client\marionette_driver\wait.py", line 150, in until
    cause=last_exc)
TimeoutException: Timed out after 30.0 seconds with message: got button1 instead of button1-mousemove-mousedown
 2:49.38 TEST_START: MainThread test_single_finger_desktop.py testSingleFingerMouse.test_press_release
 3:19.72 TEST_END: MainThread ERROR, expected PASS
Traceback (most recent call last):
  File "c:\mozilla-source\mozilla-central\testing/marionette/harness\marionette_harness\marionette_test\testcases.py", line 166, in run
    testMethod()
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\test_single_finger_desktop.py", line 71, in test_press_release
    press_release(self.marionette, 1, "button1-mousemove-mousedown-mouseup-click")
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\single_finger_functions.py", line 23, in press_release
    wait_for_condition_else_raise(marionette, expected, "return document.getElementById('button1').innerHTML;")
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\single_finger_functions.py", line 9, in wait_for_condition_else_raise
    message="got {0} instead of {1}".format(marionette.execute_script(script), expected),
  File "c:\mozilla-source\mozilla-central\testing/marionette/client\marionette_driver\wait.py", line 150, in until
    cause=last_exc)
TimeoutException: Timed out after 30.1 seconds with message: got button1-touchstart-touchend-mousemove-mousedown-mouseup-click instead of button1-mousemove-mousedown-mouseup-click
 3:19.75 TEST_START: MainThread test_single_finger_desktop.py testSingleFingerMouse.test_press_release_twice
 3:50.24 TEST_END: MainThread ERROR, expected PASS
Traceback (most recent call last):
  File "c:\mozilla-source\mozilla-central\testing/marionette/harness\marionette_harness\marionette_test\testcases.py", line 166, in run
    testMethod()
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\test_single_finger_desktop.py", line 74, in test_press_release_twice
    press_release(self.marionette, 2, "button1-mousemove-mousedown-mouseup-click-mousemove-mousedown-mouseup-click")
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\single_finger_functions.py", line 23, in press_release
    wait_for_condition_else_raise(marionette, expected, "return document.getElementById('button1').innerHTML;")
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\single_finger_functions.py", line 9, in wait_for_condition_else_raise
    message="got {0} instead of {1}".format(marionette.execute_script(script), expected),
  File "c:\mozilla-source\mozilla-central\testing/marionette/client\marionette_driver\wait.py", line 150, in until
    cause=last_exc)
TimeoutException: Timed out after 30.1 seconds with message: got button1-touchstart-touchend-mousemove-mousedown-mouseup-click-touchstart-touchend-mousemove-mousedown-mouseup-click instead of button1-mousemove-mousedown-mouseup-click-mousemove-mousedown-mouseup-click
 3:50.27 TEST_START: MainThread test_single_finger_desktop.py testSingleFingerMouse.test_single_tap
 4:20.55 TEST_END: MainThread ERROR, expected PASS
Traceback (most recent call last):
  File "c:\mozilla-source\mozilla-central\testing/marionette/harness\marionette_harness\marionette_test\testcases.py", line 166, in run
    testMethod()
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\test_single_finger_desktop.py", line 120, in test_single_tap
    single_tap(self.marionette, "button1-mousemove-mousedown-mouseup-click")
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\single_finger_functions.py", line 122, in single_tap
    wait_for_condition_else_raise(marionette, expected, "return document.getElementById('button1').innerHTML;")
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\single_finger_functions.py", line 9, in wait_for_condition_else_raise
    message="got {0} instead of {1}".format(marionette.execute_script(script), expected),
  File "c:\mozilla-source\mozilla-central\testing/marionette/client\marionette_driver\wait.py", line 150, in until
    cause=last_exc)
TimeoutException: Timed out after 30.0 seconds with message: got button1-touchstart-touchend-mousemove-mousedown-mouseup-click instead of button1-mousemove-mousedown-mouseup-click
 4:20.58 TEST_START: MainThread test_single_finger_desktop.py testSingleFingerMouse.test_wait
 4:50.94 TEST_END: MainThread ERROR, expected PASS
Traceback (most recent call last):
  File "c:\mozilla-source\mozilla-central\testing/marionette/harness\marionette_harness\marionette_test\testcases.py", line 166, in run
    testMethod()
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\test_single_finger_desktop.py", line 83, in test_wait
    wait(self.marionette, "button1-mousemove-mousedown-mouseup-click")
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\single_finger_functions.py", line 74, in wait
    wait_for_condition_else_raise(marionette, expected, "return document.getElementById('button1').innerHTML;")
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\single_finger_functions.py", line 9, in wait_for_condition_else_raise
    message="got {0} instead of {1}".format(marionette.execute_script(script), expected),
  File "c:\mozilla-source\mozilla-central\testing/marionette/client\marionette_driver\wait.py", line 150, in until
    cause=last_exc)
TimeoutException: Timed out after 30.1 seconds with message: got button1-touchstart-touchend-mousemove-mousedown-mouseup-click instead of button1-mousemove-mousedown-mouseup-click
 4:50.97 TEST_START: MainThread test_single_finger_desktop.py testSingleFingerMouse.test_wait_with_value
 5:21.35 TEST_END: MainThread ERROR, expected PASS
Traceback (most recent call last):
  File "c:\mozilla-source\mozilla-central\testing/marionette/harness\marionette_harness\marionette_test\testcases.py", line 166, in run
    testMethod()
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\test_single_finger_desktop.py", line 86, in test_wait_with_value
    wait_with_value(self.marionette, "button1-mousemove-mousedown-mouseup-click")
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\single_finger_functions.py", line 83, in wait_with_value
    wait_for_condition_else_raise(marionette, expected, "return document.getElementById('button1').innerHTML;")
  File "c:\mozilla-source\mozilla-central\testing\marionette\harness\marionette_harness\tests\unit\single_finger_functions.py", line 9, in wait_for_condition_else_raise
    message="got {0} instead of {1}".format(marionette.execute_script(script), expected),
  File "c:\mozilla-source\mozilla-central\testing/marionette/client\marionette_driver\wait.py", line 150, in until
    cause=last_exc)
TimeoutException: Timed out after 30.0 seconds with message: got button1-touchstart-touchend-mousemove-mousedown-mouseup-click instead of button1-mousemove-mousedown-mouseup-click
 5:21.37 LOG: MainThread INFO
SUMMARY
-------
 5:21.38 LOG: MainThread INFO passed: 0
 5:21.38 LOG: MainThread INFO failed: 10
 5:21.38 LOG: MainThread INFO todo: 4 (skipped: 4)
 5:21.38 LOG: MainThread INFO
FAILED TESTS
-------
 5:21.38 LOG: MainThread INFO test_single_finger_desktop.py test_single_finger_desktop.testSingleFingerMouse.test_chain
 5:21.38 LOG: MainThread INFO test_single_finger_desktop.py test_single_finger_desktop.testSingleFingerMouse.test_chain_flick
 5:21.38 LOG: MainThread INFO test_single_finger_desktop.py test_single_finger_desktop.testSingleFingerMouse.test_double_tap
 5:21.38 LOG: MainThread INFO test_single_finger_desktop.py test_single_finger_desktop.testSingleFingerMouse.test_move_by_offset
 5:21.38 LOG: MainThread INFO test_single_finger_desktop.py test_single_finger_desktop.testSingleFingerMouse.test_move_element
 5:21.38 LOG: MainThread INFO test_single_finger_desktop.py test_single_finger_desktop.testSingleFingerMouse.test_press_release
 5:21.39 LOG: MainThread INFO test_single_finger_desktop.py test_single_finger_desktop.testSingleFingerMouse.test_press_release_twice
 5:21.39 LOG: MainThread INFO test_single_finger_desktop.py test_single_finger_desktop.testSingleFingerMouse.test_single_tap
 5:21.39 LOG: MainThread INFO test_single_finger_desktop.py test_single_finger_desktop.testSingleFingerMouse.test_wait
 5:21.39 LOG: MainThread INFO test_single_finger_desktop.py test_single_finger_desktop.testSingleFingerMouse.test_wait_with_value
 5:21.39 SUITE_END: MainThread
Summary
=======

Ran 14 tests
Expected results: 0
Unexpected results: 10 (ERROR: 10)
Skipped: 4

Unexpected Results
==================

ERROR test_single_finger_desktop.py testSingleFingerMouse.test_chain
ERROR test_single_finger_desktop.py testSingleFingerMouse.test_chain_flick
ERROR test_single_finger_desktop.py testSingleFingerMouse.test_double_tap
ERROR test_single_finger_desktop.py testSingleFingerMouse.test_move_by_offset
ERROR test_single_finger_desktop.py testSingleFingerMouse.test_move_element
ERROR test_single_finger_desktop.py testSingleFingerMouse.test_press_release
ERROR test_single_finger_desktop.py testSingleFingerMouse.test_press_release_twice
ERROR test_single_finger_desktop.py testSingleFingerMouse.test_single_tap
ERROR test_single_finger_desktop.py testSingleFingerMouse.test_wait
ERROR test_single_finger_desktop.py testSingleFingerMouse.test_wait_with_value

===================================================================

Is this a known issue ? As the changes aren't relatively close to the logic that has been implemented, so they should not be hampering the actual functionality.


Thanks,
Aseem

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Aseem, please try to not past full logs into comments. Instead make use of an attachment in the future. Thanks.

Regarding your question, it's hard to see from this log only. I would suggest that you revert changes you have made to this test module step by step until it works. It should give an indication what was wrong.

Comment 24

[Aseem Yadav]

(In reply to Henrik Skupin (:whimboo) from comment #23)
> Aseem, please try to not past full logs into comments. Instead make use of
> an attachment in the future. Thanks.
Will take care of it from now onward.

> Regarding your question, it's hard to see from this log only. I would
> suggest that you revert changes you have made to this test module step by
> step until it works. It should give an indication what was wrong.

Tried reverting the changes, but no success. The cases are still failing. Will build the source code again in order to drill down to the actual cause.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I'm not sure what the question is for me from the last comment. Were you able to work out the problem? If not, did you completely revert the changes of the mouse tests?

Comment 26

[Aseem Yadav]

(In reply to Henrik Skupin (:whimboo) from comment #25)
> I'm not sure what the question is for me from the last comment. Were you
> able to work out the problem? If not, did you completely revert the changes
> of the mouse tests?

I'm sorry if I wasn't clear. What I meant that I reverted the changes but found that the mouse tests were still failing, however, each time the expected output was a bit different. 
Now, I have built the source code again after pulling the latest changes, and can see that the cases are still failing.
So, should I file a bug for this because apparently it is blocking me from making changes for "self.wait_for_condition()" ?

(Also, I don't know if I have gained right to file a bug, but I can draft one which you can use later on to file)

Thanks,
Aseem

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I'm totally sorry in not having responded to you sooner. I somehow completely missed it first, and then I was sure that I definitely have responded. But looks like due to traveling my comment hasn't been sent out. :/

Regarding all the failures please pull and build again. Maybe the failures aren't visible anymore. If they are please do a pastebin with all the detailed output. It will help me to determine what's wrong. Thanks

Comment 28

[Aseem Yadav]

(In reply to Henrik Skupin (:whimboo) from comment #27)
> I'm totally sorry in not having responded to you sooner. I somehow
> completely missed it first, and then I was sure that I definitely have
> responded. But looks like due to traveling my comment hasn't been sent out.
> :/
No problem there.
> Regarding all the failures please pull and build again. Maybe the failures
> aren't visible anymore. If they are please do a pastebin with all the
> detailed output. It will help me to determine what's wrong. Thanks

Right to it. Let me pull the latest changes and see if those errors are still there.

Comment 29

[Aseem Yadav]

Attachment: Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

Review commit: https://reviewboard.mozilla.org/r/175750/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/175750/

Comment 30

[Aseem Yadav]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131172/diff/3-4/

Comment 31

[Aseem Yadav]

Changes have been submitted for review.
Henrik, please take a look !!

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I hopefully find the time later today to have a look at the changes. Thank you for the update!

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Aseem Yadav from comment #31)
> Changes have been submitted for review.

I'm so sorry that I totally missed this review. Usually those things do not slip through, but if it happens it might be good to ping me again maybe on IRC or here via a comment. I'm trying hard to do reviews within a day.

I will have a look at it now!

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8904003 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

https://reviewboard.mozilla.org/r/175750/#review189624

The changes to the file test_modal_dialogs.py look fine to me, but we now have two commits in this series. As I have requested earlier can you please make sure that every change which does not touch the problematic call to `wait_for_condition` in `single_finger_functions.py` gets into the same commit? It would be really good if we could finish on those files, and then have a single other commit for removing the `wait_for_condition` wrapper in `single_finger_functions.py`, and `testcase.py`.

In case of questions please let me know and we can chat on IRC. I'm happy to explain the detailed steps to do for getting this done. I know that this might not be easy.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Aseem, some months have been passed by and I would like to know if you are still interested to continue on this patch. Please let me know. Thanks.

Comment 36

[Aseem Yadav]

(In reply to Henrik Skupin (:whimboo) from comment #35)
> Aseem, some months have been passed by and I would like to know if you are
> still interested to continue on this patch. Please let me know. Thanks.

Yes Henrik, sorry that I lost track of it due to some other priorities. Will try to cover it immediately and I will be needing some help from you as well for putting this on the branch.

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Great. Best is when you reach out to me on irc.mozilla.org in the #ateam channel. There are some things which will make it easier to discuss live in that channel as via Bugzilla. Thanks.

Comment 38

[Leni Kadali [:leni1] [GMT+3]]

Hi Aseem,

Just wanted to know if you're still working on this bug. Would like to take it up.

Comment 39

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hi Leni. Yes he is. We had to figure out some Mercurial issues the last week. So hopefully sometimes this week a new set of patches can be uploaded.

Comment 40

[Aseem Yadav]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131172/diff/4-5/

Comment 41

[Code Review Bot [:reviewbot]]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

https://reviewboard.mozilla.org/r/131172/#review240424


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/harness/marionette_harness/marionette_test/testcases.py:23
(Diff revision 5)
>      _ExpectedFailure,
>      _UnexpectedSuccess,
>      SkipTest,
>  )
>  
>  from marionette_driver.errors import (

Error: 'marionette_driver.errors.timeoutexception' imported but unused [flake8: F401]

Comment 42

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

https://reviewboard.mozilla.org/r/131172/#review240730

This patch looks way better now! So it was good to learn about the squash feature. Thank you for the update.

Beside that I still had to make a couple of review comments which you will have to address. Some actually cause breakage, and as such I cannot give a r+ yet. Let me know if you have questions.

::: testing/marionette/harness/marionette_harness/tests/unit/single_finger_functions.py:9
(Diff revision 5)
>  from marionette_driver.errors import TimeoutException
>  from marionette_driver.by import By
>  
>  
> -def wait_for_condition_else_raise(marionette, wait_for_condition, expected, script):
> -    try:
> +def wait_for_condition_else_raise(marionette, expected, script):
> +    Wait(marionette, timeout=30).until(

You haven't imported `Wait`, so all the tests fail. Please make sure to also run the modified tests before submitting changes to mozreview.

::: testing/marionette/harness/marionette_harness/tests/unit/test_legacy_mouse_action.py:98
(Diff revision 5)
>          el = self.marionette.find_element(By.ID, "showbutton")
>          self.action.middle_click(el).perform()
>  
> -        self.wait_for_condition(lambda _: el.get_property("innerHTML") == "1")
> +        Wait(self.marionette).until(
> +            lambda _: el.get_property("innerHTML") == "1",
> +            message="The property 'innerHTML' does not equal '1'",

Given that a message would hide the actual values, lets better get rid of the message here and stay what originally has been used. That way we can see the value of `innerHTML`.

::: testing/marionette/harness/marionette_harness/tests/unit/test_legacy_mouse_action.py:111
(Diff revision 5)
>              urlbar.send_keys(data_uri)
>              go_button = self.marionette.execute_script("return gURLBar.goButton")
>              self.action.click(go_button).perform()
> -        self.wait_for_condition(lambda mn: mn.get_url() == data_uri)
> +        Wait(self.marionette).until(
> +            lambda mn: mn.get_url() == data_uri,
> +            message="The expected data URI page has not been loaded",

Same here.

::: testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py:82
(Diff revision 5)
>          self.wait_for_alert()
>          alert = self.marionette.switch_to_alert()
>          alert.accept()
> -        self.wait_for_condition(
> -            lambda mn: mn.find_element(By.ID, "confirm-result").text == "true")
> +        Wait(self.marionette).until(
> +            lambda mn: mn.find_element(By.ID, "confirm-result").text == "true",
> +            message="Value of element 'confirm-result' is 'false'")

`text` is not a boolean so please remove the message from all of these tests in this module so that we also see more assertion details in the log.

::: testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py:189
(Diff revision 5)
>          self.marionette.find_element(By.ID, "onbeforeunload-handler").click()
> -        self.wait_for_condition(
> +        Wait(self.marionette).until(
>              lambda mn: mn.execute_script("""
>                return window.onbeforeunload !== null;
> -            """))
> +            """),
> +            message="Handler for 'onbeforeunload' has not been set")

The message for the handler is actually fine because we have only two states.

::: testing/marionette/harness/marionette_harness/tests/unit/test_single_finger_desktop.py:14
(Diff revision 5)
>  
>  from marionette_driver.errors import MarionetteException
>  from marionette_driver import Actions, By
>  
>  from marionette_harness import MarionetteTestCase, skip
> +from marionette_driver.wait import Wait

It's not required in this file.

Comment 43

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Aseem, do you have an update for us? Thanks.

Comment 44

[Aseem Yadav]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

https://reviewboard.mozilla.org/r/131172/#review253564

::: testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py:82
(Diff revision 5)
>          self.wait_for_alert()
>          alert = self.marionette.switch_to_alert()
>          alert.accept()
> -        self.wait_for_condition(
> -            lambda mn: mn.find_element(By.ID, "confirm-result").text == "true")
> +        Wait(self.marionette).until(
> +            lambda mn: mn.find_element(By.ID, "confirm-result").text == "true",
> +            message="Value of element 'confirm-result' is 'false'")

ok, removed the argument "message" from the cases that look for boolean value.

Comment 45

[Aseem Yadav]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131172/diff/5-6/

Comment 46

[Code Review Bot [:reviewbot]]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

https://reviewboard.mozilla.org/r/131172/#review254008


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/harness/marionette_harness/marionette_test/testcases.py:23
(Diff revision 6)
>      _ExpectedFailure,
>      _UnexpectedSuccess,
>      SkipTest,
>  )
>  
>  from marionette_driver.errors import (

Error: 'marionette_driver.errors.timeoutexception' imported but unused [flake8: F401]

Comment 47

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

https://reviewboard.mozilla.org/r/131172/#review240424

> Error: 'marionette_driver.errors.timeoutexception' imported but unused [flake8: F401]

Please make sure to close this issue if you fixed it. I cannot do it myself for the code review bot. Thanks.

Comment 48

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8859126 [details]
Bug 1354458 - wait_for_condition() now replaced with wait().until() in unit tests

https://reviewboard.mozilla.org/r/131172/#review254612

The patch looks to be nearly done. Please check the review comments. Especially the last one needs to be fixed before we can merge the patch.

I also noticed that we haven't run a try build yet. So I will trigger one yet, so that we can see the results for the changes across platforms.

::: testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py:82
(Diff revisions 5 - 6)
>          self.wait_for_alert()
>          alert = self.marionette.switch_to_alert()
>          alert.accept()
>          Wait(self.marionette).until(
>              lambda mn: mn.find_element(By.ID, "confirm-result").text == "true",
> -            message="Value of element 'confirm-result' is 'false'")
> +            )

Please put the closing bracket into the same column as the `W` in `Wait`. It also applies to the entries below.

::: testing/marionette/harness/marionette_harness/marionette_test/testcases.py:24
(Diff revision 6)
>      _UnexpectedSuccess,
>      SkipTest,
>  )
>  
>  from marionette_driver.errors import (
>      TimeoutException,

This is still imported in the latest patch, but not used anywhere. Please only mark issues fixed once you really fixed them.

::: testing/marionette/harness/marionette_harness/tests/unit/single_finger_functions.py:11
(Diff revision 6)
> +from marionette_driver.wait import Wait
>  
>  
> -def wait_for_condition_else_raise(marionette, wait_for_condition, expected, script):
> -    try:
> -        wait_for_condition(lambda m: expected in m.execute_script(script))
> +def wait_for_condition_else_raise(marionette, expected, script):
> +    Wait(marionette, timeout=30).until(
> +            lambda m: expected in m.execute_script(script),

nit: please use an indentation of 4 chars for both lines. The closing bracket should also be in column 1.

::: testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py:185
(Diff revision 6)
> +            message="Value of element 'prompt-result' is not null")
>  
>      def test_onbeforeunload_dismiss(self):
>          start_url = self.marionette.get_url()
>          self.marionette.find_element(By.ID, "onbeforeunload-handler").click()
> -        self.wait_for_condition(
> +        Wait(self.marionette).until(

Please rebase your patch against current mozilla-central. The tests for onbeforeunload are no longer present in this file and will cause a merge conflict.

Comment 49

[Henrik Skupin [:whimboo][⌚️UTC+1]]

What we actually missed to check if other harnesses which are based on Marionette also make use of that method. And it looks like that way:

https://dxr.mozilla.org/mozilla-central/search?q=file%3A*.py+wait_for_condition&redirect=false

Sorry, but you would also have to update those other files.

Comment 50

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Please note the test failures in this try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=30066bd7d52e54afecd4418d2619e4b8f4ccfa19

It means that you are working on a very outdated changeset of mozilla-central. You might want to pull and rebase your working branch. Maybe there are also some conflicts to solve.

Comment 51

[Adrian Kaczmarek]

Attachment: 1354458

Replaced some occurrences of the self.wait_for_condition function

Comment 52

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Adrian, thank you for the diff but it looks like you started from fresh. Instead it would be very helpful if you could check attachment 8859126 [details] and take those changes as base for a final patch. Then apply my review comments from comment 48 on top of it. 

Will you have the time to do that?

Comment 53

[Agboola Mukhtar <agbo-ola>]

Attachment: Bug 1354458 - wait_for_condition() replaced with wait().until() in unit tests as referenced from Aseem's PR

Implemented all what Aseem's previously implemented and the other changes @whimboo suggested.

Comment 54

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Fixing this bug got way simpler due to some code removals in the past months. If someone wants to have a look at it, I'm happy to still mentor.

Comment 55

[Aaditya]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #54)

Fixing this bug got way simpler due to some code removals in the past months. If someone wants to have a look at it, I'm happy to still mentor.

Hi,
Is the issue still open?

Comment 56

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Given that this is an outstanding issue we would like to get fixed soon, I already asked Rishi yesterday to take care of it. He just haven't had the time to reply here.

Aaditya, if you are interested in a good first bug - and I see you just joined Bugzilla - I would propose bug 1583504 to you. If you want to take it just comment over there, and we can get you started. Thanks

Comment 57

[Aaditya]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #56)

Given that this is an outstanding issue we would like to get fixed soon, I already asked Rishi yesterday to take care of it. He just haven't had the time to reply here.

Aaditya, if you are interested in a good first bug - and I see you just joined Bugzilla - I would propose bug 1583504 to you. If you want to take it just comment over there, and we can get you started. Thanks

Ok. I will comment over there.
Thank you.

Comment 58

[Rishi Gupta (:rgpt)]

Thank you : whimboo. I will start working on this enhancement.

Comment 59

[Rishi Gupta (:rgpt)]

Attachment: bug 1354458 - Replacing self.wait_for_condition() with Wait().until() function

Comment 60

[Rishi Gupta (:rgpt)]

Attachment: bug 1354458 - Replacing self.wait_for_condition() with Wait().until() function

Comment 61

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Rishi, with the latest push you added again a new revision of your patch. Not sure which command you used, but please really update the original revision instead. Therefore check that the commit message contains a reference to the phabricator revision, which should have been added by moz-phab by default when you did the first push. Successive pushes will just update that same revision.

Comment 62

[Rishi Gupta (:rgpt)]

:whimboo, I created a separate revision for the patch using 'arc diff --create' for the review. So if there is a fold required then it should come in a separate revision apart from that same revision is to be updated. From now on i will update in the latest revision itself and can mark the first revision https://phabricator.services.mozilla.com/D47533 as abandon ?

Comment 63

[Henrik Skupin [:whimboo][⌚️UTC+1]]

No, please do not use arc directly, but moz-phab submit to submit all the commits of a bookmark. It will take care automatically if revisions have to be created or just updated. It also keep the review flow in-tact. For this time please abandon D47533 yes, but lets make sure to not have such a situation again. Also because I already mentioned it on the last bug you were working on. Thanks.

Comment 64

[Rishi Gupta (:rgpt)]

@whimboo, i have made modifications as discussed with you. Also, i will make sure to use moz-phab instead of arcanist directly.

Comment 65

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ebeb78d6ad37
Replacing self.wait_for_condition() with Wait().until() function r=whimboo

Comment 66

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/ebeb78d6ad37