1334996 - Intermittent test_safe_browsing_warning_pages.py TestSafeBrowsingWarningPages.test_warning_pages | AssertionError: phishing-malware != how-does-phishing-and-malware-protection- [truncated]...

Status: RESOLVED FIXED

(Testing :: Firefox UI Tests, defect)

Description

[Treeherder Bug Filer]

Filed by: archaeopteryx [at] coole-files.de

https://treeherder.mozilla.org/logviewer.html#?job_id=72954024&repo=mozilla-inbound

https://queue.taskcluster.net/v1/task/QyrcMYpYRNKnkGctP21Jmw/runs/0/artifacts/public/logs/live_backing.log

From IRC: whimboo: oh, we do assertEquals() while it should be a Wait().until().

Comment 1

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

The code in question is here:

https://dxr.mozilla.org/mozilla-central/rev/71224049c0b52ab190564d3ea0eab089a159a4cf/testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py#92-100

We should remove the content of the until() call, and move in the check for assertEquals instead.

Comment 2

[Rahul Jha [:RJ7]]

(In reply to Henrik Skupin (:whimboo) from comment #1)
> The code in question is here:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 71224049c0b52ab190564d3ea0eab089a159a4cf/testing/firefox-ui/tests/functional/
> security/test_safe_browsing_warning_pages.py#92-100
> 
> We should remove the content of the until() call, and move in the check for
> assertEquals instead.

Hi, I would like to work on this bug

What I understood is that I shall remove the lambda function inside the until (because the lambda function is not even executed) and should replace it with the assertEquals statement below.

Comment 3

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

Hi Rahul, great to hear that you are interested in this bug!

Regarding your question, the lambda of the until() call has to be replaced with the check as currently done in the assertEquals. That is because this check is more specific, and we should wait for.

Thanks for working on it!

Comment 4

[Rahul Jha [:RJ7]]

Thanks for helping out @Henrik
Now I get it, but I cannot figure out how to download the code:

I tried ```hg clone https://hg.mozilla.org/mozilla-central src```

But after downloading , it shows an error message: 

```
transaction abort!                                                                                               
rollback completed
abort: stream ended unexpectedly (got 131068 bytes, expected 1953854053)
```
(hg version = 4.1 and OS = Ubuntu 14.04)

I am unfamiliar with hg and don't know to configure it, do I need a Mozilla-registered LDAP account that is configured for SSH access to Mercurial for the purpose or there is some other method.

What else do you think that I need to know before contributing to mozilla?
Thanks in advance :)

Comment 5

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

Rahul, it looks like there have been networking issues for you. And that quiet early in the process of cloning the repository. There is no specific LDAP account necessary.

Maybe have a look at the following documentation which offers a better way to work with the repo:
http://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/unifiedrepo.html

When you have made the changes please make sure to also run the Marionette test:
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/Running_Tests

Let me know if you need anything else. If I'm not around on IRC please join us in #ateam and just ask. Others will be able to also help you out.

Comment 6

[Anjul Tyagi]

Can I work on this one ??

Comment 7

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

Rahul, are you already working on this bug and want to provide a patch? If not please tell us.

@Anjul, please wait until we got a response from Rahul, or have a look at other bugs like bug 1336022, which kinda similar to this case.

Comment 8

[Rahul Jha [:RJ7]]

@whimboo, I am working on the bug and hopefully would submit a patch by Tuesday :)

Comment 9

[Intermittent Failures Robot]

9 failures in 733 pushes (0.012 failures/push) were associated with this bug in the last 7 days.  

Repository breakdown:
* mozilla-inbound: 7
* autoland: 2

Platform breakdown:
* linux64: 5
* linux32: 3
* android-4-3-armv7-api15: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1334996&startday=2017-01-30&endday=2017-02-05&tree=all

Comment 10

[Rahul Jha [:RJ7]]

Attachment: patch.diff

Hello @whimboo

I am sorry for late submission, had some network problem - the file size was huge and couldn't be downloaded.

So, what I did was - I downloaded the zip file from https://github.com/mozilla/gecko-dev , but as I came to know for commit history and all git related stuff, I would have to clone it, so I just copied the file: test_safe_browsing_warning_pages.py in an empty folder, initialised it as git repo and then made the changes, and generated the patch file. I don't know if I have done it the right way?

Comment 11

[Aseem Yadav]

Hi,
Can you assign me this bug ? I have actually worked my way around and found a solution that rectifies the issue.

Comment 12

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

Hi Aseem, so Rahul is already working on this bug and uploaded a patch recently. Because of that it would be great if you could check for another bug you could work on. Thanks.

Rahul, in the future it's not necessary to set all three flags (ni?, f?, and r?). All have a different meaning. ni? is for general questions, while f? means have a look at my work in progress patch. r? is finally the review of the patch. 

I will have a look at the patch soon.

Comment 13

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

Comment on attachment 8835876 [details] [diff] [review]
patch.diff

Review of attachment 8835876 [details] [diff] [review]:
-----------------------------------------------------------------

Please have a look at the inline comment. The patch needs a substantial change. 

Beside that please make sure to update the commit message, so it contains only helpful details why this change is necessary.

::: test_safe_browsing_warning_pages.py
@@ +92,5 @@
>          # Wait for page load to be completed, so we can verify the URL even if a redirect happens.
>          # TODO: Bug 1140470: use replacement for mozmill's waitforPageLoad
>          Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> +            # check that our current url matches the final url we expect
> +            self.assertEquals(self.marionette.get_url(), self.browser.get_final_url(url))

You cannot simply copy the assertEquals() call into until. The latter expects a callback or a lambda, which you want to use here. Also the comment you placed into line 95, should be the message parameter of until().

Comment 14

[Anjul Tyagi]

Are you still working on this one ?

Comment 15

[Aseem Yadav]

(In reply to Henrik Skupin (:whimboo) [away 02/18 - 02/27] from comment #13)
> Comment on attachment 8835876 [details] [diff] [review]
> patch.diff
> 
> Review of attachment 8835876 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please have a look at the inline comment. The patch needs a substantial
> change. 
> 
> Beside that please make sure to update the commit message, so it contains
> only helpful details why this change is necessary.
> 
> ::: test_safe_browsing_warning_pages.py
> @@ +92,5 @@
> >          # Wait for page load to be completed, so we can verify the URL even if a redirect happens.
> >          # TODO: Bug 1140470: use replacement for mozmill's waitforPageLoad
> >          Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> > +            # check that our current url matches the final url we expect
> > +            self.assertEquals(self.marionette.get_url(), self.browser.get_final_url(url))
> 
> You cannot simply copy the assertEquals() call into until. The latter
> expects a callback or a lambda, which you want to use here. Also the comment
> you placed into line 95, should be the message parameter of until().

@whimboo mind if I submit the patch now ? I have already verified it.

Comment 16

[Rahul Jha [:RJ7]]

(In reply to Anjul Tyagi from comment #14)
> Are you still working on this one ?

I am sorry, I was out of station for a while, and that's why couldn't reply!
I am currently working on it and will submit a patch today hopefully! :)

Comment 17

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

Looks like another week passed by without an update for the patch. Aseem, if you have a working patch around please submit via mozreview. Thanks.

Comment 18

[Aseem Yadav]

Attachment: Intermittent test_safe_browsing_warning_pages.py patch (bug 1334996)

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

Comment 19

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

Comment on attachment 8842028 [details]
Intermittent test_safe_browsing_warning_pages.py patch (bug 1334996)

https://reviewboard.mozilla.org/r/116046/#review117526

If you want your patch being reviewed please remember to set the reviewer flag the next time you have updated the patch. Simply add my nickname and it will be taken.

Otherwise please the inline comments.

::: testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py:99
(Diff revision 1)
> -        Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> -            lambda mn: mn.execute_script('return document.readyState == "DOMContentLoaded" ||'
> -                                         '       document.readyState == "complete";')
> -        )
> +        #Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> +        #    lambda mn: mn.execute_script('return document.readyState == "DOMContentLoaded" ||'
> +        #                                 '       document.readyState == "complete";')
> +        #)
> +        if not self.wait_url(self.marionette.get_url(), self.browser.get_final_url(url)):
> +            exit()

First, you should never push patches with some code commented out. It should all be removed.

Second, why did you implement a different `wait_url()` method? Wait().until() is already perfect and as I mentioned before the content of the next assert line has to simply be moved into it.

Comment 20

[Aseem Yadav]

Comment on attachment 8842028 [details]
Intermittent test_safe_browsing_warning_pages.py patch (bug 1334996)

https://reviewboard.mozilla.org/r/116046/#review117526

Will take care of it next time. It was the first time I was pushing changes using mercurial, so it got me confused.

Comment 21

[Aseem Yadav]

Attachment: Issue fix Bug 1334996

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

Comment 22

[Aseem Yadav]

Attachment: patch to intermittent issue (Bug 1334996);

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

Comment 23

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

Comment on attachment 8843837 [details]
patch to intermittent issue (Bug 1334996);

https://reviewboard.mozilla.org/r/116690/#review119580

This version of the patch looks already better. Thank you for the update. Please see my inline comments in what needs to be fixed still.

::: commit-message-eb236:1
(Diff revision 1)
> +patch to intermittent issue (Bug 1334996); r?whimboo

We definitely need a better commit message for this fix so that it is clear what we are getting fixed. Also please follow the formatting of the message as described on the web page I gave you lately.

::: testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py:96
(Diff revision 1)
>  
>          # Wait for page load to be completed, so we can verify the URL even if a redirect happens.
>          # TODO: Bug 1140470: use replacement for mozmill's waitforPageLoad
> +
>          Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> -            lambda mn: mn.execute_script('return document.readyState == "DOMContentLoaded" ||'
> +            lambda mn: True if mn.get_url() == self.browser.get_final_url(url) else False ,

The equal comparison already results in a boolean value. So there is no need to wrap everything in another if condition.

::: testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py:97
(Diff revision 1)
>          # Wait for page load to be completed, so we can verify the URL even if a redirect happens.
>          # TODO: Bug 1140470: use replacement for mozmill's waitforPageLoad
> +
>          Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> -            lambda mn: mn.execute_script('return document.readyState == "DOMContentLoaded" ||'
> -                                         '       document.readyState == "complete";')
> +            lambda mn: True if mn.get_url() == self.browser.get_final_url(url) else False ,
> +            message= "The current url did not match with the final url we expected"

In the message we have to add which final url we expect. This will help starring the failure on Treeherder if it occurs.

Comment 24

[Aseem Yadav]

Comment on attachment 8843837 [details]
patch to intermittent issue (Bug 1334996);

https://reviewboard.mozilla.org/r/116690/#review119580

> We definitely need a better commit message for this fix so that it is clear what we are getting fixed. Also please follow the formatting of the message as described on the web page I gave you lately.

Hope this would do.
"Bug 1334996 - removed assert declaration, added URL equality check to wait()"

> The equal comparison already results in a boolean value. So there is no need to wrap everything in another if condition.

Done.

> In the message we have to add which final url we expect. This will help starring the failure on Treeherder if it occurs.

message=Expected URL ['{0}']\ndid not match with the Captured URL : ['{1}'].

As there are 3 checks, So I think showing captured URL will help in knowing which one failed.

Comment 25

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

Comment on attachment 8843837 [details]
patch to intermittent issue (Bug 1334996);

https://reviewboard.mozilla.org/r/116690/#review119580

> Hope this would do.
> "Bug 1334996 - removed assert declaration, added URL equality check to wait()"

Lets use: `Bug 1334996 - Click on Safebrowsing report button has to wait for the final URL.`.

> Done.

You haven't pushed an update to mozreview yet, so I cannot verify it.

> message=Expected URL ['{0}']\ndid not match with the Captured URL : ['{1}'].
> 
> As there are 3 checks, So I think showing captured URL will help in knowing which one failed.

No, there is only the final URL we have to wait for, and nothing else. See bug 1344647 for an example of a assertion message.

Comment 26

[Aseem Yadav]

Attachment: Bug 1334996 - Click on Safebrowsing report button has to wait for the final URL.

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

Comment 27

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

Comment on attachment 8845744 [details]
Bug 1334996 - Click on Safebrowsing report button has to wait for the final URL.

https://reviewboard.mozilla.org/r/118884/#review120904

Now you got the same patch added twice to the patch series. Actually I'm not sure how this is possible. But please check locally with `hg histedit` if you are using Mercurial, and only keep the the commit which we really want.

Comment 28

[Aseem Yadav]

Attachment: Bug 1334996 - Click on Safebrowsing report button has to wait for the final URL.

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

Comment 29

[Aseem Yadav]

Made the changes, striped off the ambiguous commits to the actual working change.
Awaiting your response to the review.

Comment 30

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

Comment on attachment 8845892 [details]
Bug 1334996 - Click on Safebrowsing report button has to wait for the final URL.

https://reviewboard.mozilla.org/r/119018/#review122998

r+ with the comments implemented.

::: testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py:96
(Diff revision 1)
>  
>          # Wait for page load to be completed, so we can verify the URL even if a redirect happens.
>          # TODO: Bug 1140470: use replacement for mozmill's waitforPageLoad
>          Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> -            lambda mn: mn.execute_script('return document.readyState == "DOMContentLoaded" ||'
> -                                         '       document.readyState == "complete";')
> +            lambda mn: mn.get_url() == self.browser.get_final_url(url) ,
> +            message= "The Expected URL '{}' has not been loaded".format(self.browser.get_final_url(url))

Just some nits... Don't use a space after the `=` sign for parameters. Further please change `Expected` to `expected`.

To shorten the lines and also that we have to run `get_final_url()` only once, please create a local variable before line 94.

Comment 31

[Aseem Yadav]

Comment on attachment 8845892 [details]
Bug 1334996 - Click on Safebrowsing report button has to wait for the final URL.

https://reviewboard.mozilla.org/r/119018/#review122998

> Just some nits... Don't use a space after the `=` sign for parameters. Further please change `Expected` to `expected`.
> 
> To shorten the lines and also that we have to run `get_final_url()` only once, please create a local variable before line 94.

## Added a local var
        exp_url=self.browser.get_final_url(url)
## To be used across the wait() statement
        Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
            lambda mn: mn.get_url()==exp_url,
            message="The expected URL '{}' has not been loaded".format(exp_url)
        )
Plus: taken care of the nits

Comment 32

[Intermittent Failures Robot]

32 failures in 898 pushes (0.036 failures/push) were associated with this bug in the last 7 days. 

This is the #48 most frequent failure this week.  

** This failure happened more than 30 times this week! Resolving this bug is a high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 2 weeks, the affected test(s) may be disabled. ** 

Repository breakdown:
* mozilla-inbound: 16
* autoland: 16

Platform breakdown:
* linux64: 23
* linux32: 9

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1334996&startday=2017-03-20&endday=2017-03-26&tree=all

Comment 33

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Henrik, can you check what's going on with the new failures, please? There were mass failures on Friday, later passes and then again mass failures (SUMO changes)? Thank you.

Comment 34

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

Comment on attachment 8845892 [details]
Bug 1334996 - Click on Safebrowsing report button has to wait for the final URL.

https://reviewboard.mozilla.org/r/119018/#review122998

> ## Added a local var
>         exp_url=self.browser.get_final_url(url)
> ## To be used across the wait() statement
>         Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
>             lambda mn: mn.get_url()==exp_url,
>             message="The expected URL '{}' has not been loaded".format(exp_url)
>         )
> Plus: taken care of the nits

Aseem, I do not see those updates reflected in the patch on mozreview. Did you actually push those? 

> exp_url=self.browser.get_final_url(url)

Please make sure to use spaces correctly. This also applies to the equal check in the lambda method for Wait.until().

Comment 35

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

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #33)
> Henrik, can you check what's going on with the new failures, please? There
> were mass failures on Friday, later passes and then again mass failures
> (SUMO changes)? Thank you.

This might have been networking issues. I would prefer to have this patch landed sooner than later. Aseem, would you be able to finish it? Thanks.

One other thing which could have happened are SUMO outages due to pushes to production. Sebastian, do we know how long those failure windows span? Maybe we could ask a SUMO guy for feedback if such things were happening.

Comment 36

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The first failures on Friday were for this push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=59db25d0260b31bfb8f9d5a0c154f38a9e44911d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=success&filter-searchStr=en-US
They ended with https://treeherder.mozilla.org/logviewer.html#?job_id=86334398&repo=autoland

Then they resumed 10 hours later, and then went away returned etc. See https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=success&fromchange=565c6c66205d03bfd64e5fbb0a0a5a110a93c1f3&filter-searchStr=en-US&tochange=66c5bed82ee6e0677902416c1c79676f4d3059e9

Comment 37

[Aseem Yadav]

Comment on attachment 8845892 [details]
Bug 1334996 - Click on Safebrowsing report button has to wait for the final URL.

https://reviewboard.mozilla.org/r/119018/#review126674

Made the appropriate changes, will close this review so that the new changes will not get pushed to an older branch.

Comment 38

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

Comment on attachment 8845892 [details]
Bug 1334996 - Click on Safebrowsing report button has to wait for the final URL.

https://reviewboard.mozilla.org/r/119018/#review126674

No, please simply commit the changes with `hg commit --amend` so that they are folded into the former commit. And then please do not miss to run `hg push review`. So far I don't see that the patch got updated yet. Thanks.

Comment 39

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

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #36)
> Then they resumed 10 hours later, and then went away returned etc. See

What I find strange on those failures is that whenever we have those failures the current URL is https://support.mozilla.org/t5/-/-/ta-p/9395. Maybe there is a multiple redirect in place which didn't work at this time.

So the patch on this bug should definitely help here!

Comment 40

[Aseem Yadav]

Comment on attachment 8845892 [details]
Bug 1334996 - Click on Safebrowsing report button has to wait for the final URL.

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

Comment 41

[Aseem Yadav]

Comment on attachment 8845892 [details]
Bug 1334996 - Click on Safebrowsing report button has to wait for the final URL.

https://reviewboard.mozilla.org/r/119018/#review126674

The "--amend" command didn't work just as I told you earlier.
But, pushed the changes anyway.

Comment 42

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

Comment on attachment 8845892 [details]
Bug 1334996 - Click on Safebrowsing report button has to wait for the final URL.

https://reviewboard.mozilla.org/r/119018/#review126706

::: testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py:94
(Diff revision 2)
>          Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
>              expected.element_stale(button))
>  
>          # Wait for page load to be completed, so we can verify the URL even if a redirect happens.
>          # TODO: Bug 1140470: use replacement for mozmill's waitforPageLoad
> +        exp_url = self.browser.get_final_url(url)

Please rename the variable so that it is understandable. `expected_url` is way better here.

::: testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py:96
(Diff revision 2)
>  
>          # Wait for page load to be completed, so we can verify the URL even if a redirect happens.
>          # TODO: Bug 1140470: use replacement for mozmill's waitforPageLoad
> +        exp_url = self.browser.get_final_url(url)
>          Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> -            lambda mn: mn.execute_script('return document.readyState == "DOMContentLoaded" ||'
> +            lambda mn: exp_url in mn.get_url(),            

Why did you replace the `==` comparision with `in`? We clearly want to have the exact same URL here.

Please note that there are also trailing spaces in line 96 you want to remove. Maybe best is to setup your editor to do that automatically on save.

Comment 43

[Aseem Yadav]

Comment on attachment 8845892 [details]
Bug 1334996 - Click on Safebrowsing report button has to wait for the final URL.

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

Comment 44

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

Comment on attachment 8845892 [details]
Bug 1334996 - Click on Safebrowsing report button has to wait for the final URL.

https://reviewboard.mozilla.org/r/119018/#review126762

::: testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py:98
(Diff revision 3)
>          # TODO: Bug 1140470: use replacement for mozmill's waitforPageLoad
> +        expected_url = self.browser.get_final_url(url)
>          Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> -            lambda mn: mn.execute_script('return document.readyState == "DOMContentLoaded" ||'
> -                                         '       document.readyState == "complete";')
> +            lambda mn: expected_url == mn.get_url(),
> +            message="The expected URL '{}' has not been loaded".format(expected_url)
> -        )
> +         )

small nit: Please revert this indentation change for line 98. Once done it looks fine now and we can push. Thanks.

Comment 45

[Aseem Yadav]

Comment on attachment 8845892 [details]
Bug 1334996 - Click on Safebrowsing report button has to wait for the final URL.

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

Comment 46

[Aseem Yadav]

Comment on attachment 8845892 [details]
Bug 1334996 - Click on Safebrowsing report button has to wait for the final URL.

https://reviewboard.mozilla.org/r/119018/#review126772

::: testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py:98
(Diff revision 3)
>          # TODO: Bug 1140470: use replacement for mozmill's waitforPageLoad
> +        expected_url = self.browser.get_final_url(url)
>          Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> -            lambda mn: mn.execute_script('return document.readyState == "DOMContentLoaded" ||'
> -                                         '       document.readyState == "complete";')
> +            lambda mn: expected_url == mn.get_url(),
> +            message="The expected URL '{}' has not been loaded".format(expected_url)
> -        )
> +         )

Don't know how it happened, but yeah,I reverted it back.
Thanks.

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01c965ef7036
Click on Safebrowsing report button has to wait for the final URL. r=whimboo

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/01c965ef7036

Comment 49

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

test-only change which we want to see uplifted to aurora and beta, and maybe even esr52 later.

Comment 50

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

Thank you Aseem for finishing the patch off!

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d2fe539270c9

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6f8b65d518c4

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/b7807c2c30e9

Comment 54

[Aseem Yadav]

(In reply to Henrik Skupin (:whimboo) from comment #50)
> Thank you Aseem for finishing the patch off!

Pleasure.
Also, I'll be looking for more tasks related to Python automation, but if in case you come across with something you want me to handle, then feel free to assign me.

Comment 55

[Intermittent Failures Robot]

8 failures in 845 pushes (0.009 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 6
* mozilla-inbound: 2

Platform breakdown:
* linux64: 5
* linux32: 3

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1334996&startday=2017-03-27&endday=2017-04-02&tree=all

Comment 56

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

(In reply to Aseem Yadav from comment #54)
> Also, I'll be looking for more tasks related to Python automation, but if in
> case you come across with something you want me to handle, then feel free to
> assign me.

Great. So there is bug 1145112 which is a bit related to the patch you already did on this bug. Would you be interested in it? If yes, please comment on it. Thanks!