Closed Bug 1334996 Opened 3 years ago Closed 3 years ago

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

Categories

(Testing :: Firefox UI Tests, defect)

Version 3
defect
Not set

Tracking

(firefox-esr52 fixed, firefox53 fixed, firefox54 fixed, firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: justaseem51, Mentored)

Details

(Keywords: good-first-bug, intermittent-failure, Whiteboard: [lang=py][stockwell fixed])

Attachments

(2 files, 4 obsolete files)

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().
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.
Mentor: hskupin
Keywords: good-first-bug
Whiteboard: [lang=py]
(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.
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!
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 :)
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.
Can I work on this one ??
Flags: needinfo?(hskupin)
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.
Flags: needinfo?(hskupin)
Flags: needinfo?(rahul722j)
@whimboo, I am working on the bug and hopefully would submit a patch by Tuesday :)
Flags: needinfo?(rahul722j)
Attached patch patch.diffSplinter Review
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?
Flags: needinfo?(hskupin)
Attachment #8835876 - Flags: review?(hskupin)
Attachment #8835876 - Flags: feedback?(hskupin)

Hi,
Can you assign me this bug ? I have actually worked my way around and found a solution that rectifies the issue.
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.
Assignee: nobody → rahul722j
Status: NEW → ASSIGNED
Flags: needinfo?(hskupin)
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().
Attachment #8835876 - Flags: review?(hskupin)
Attachment #8835876 - Flags: review-
Attachment #8835876 - Flags: feedback?(hskupin)
Are you still working on this one ?
Flags: needinfo?(rahul722j)
(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.
(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! :)
Flags: needinfo?(rahul722j)
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 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.
Attachment #8842028 - Flags: review-
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.
Attachment #8842028 - Attachment is obsolete: true
Attachment #8842937 - Attachment is obsolete: true
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.
Attachment #8843837 - Flags: review?(hskupin) → review-
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 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.
Attachment #8845744 - Flags: review?(hskupin)
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.
Attachment #8845744 - Flags: review?(hskupin)
Attachment #8843837 - Attachment is obsolete: true
Attachment #8845744 - Attachment is obsolete: true
Attachment #8845892 - Flags: review?(hskupin)
Made the changes, striped off the ambiguous commits to the actual working change.
Awaiting your response to the review.
Flags: needinfo?(hskupin)
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.
Attachment #8845892 - Flags: review?(hskupin) → review+
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
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 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().
(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.
Assignee: rahul722j → justaseem51
Flags: needinfo?(justaseem51)
Flags: needinfo?(hskupin)
Flags: needinfo?(aryx.bugmail)
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 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.
(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 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 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.
Attachment #8845892 - Flags: review+ → review-
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.
Attachment #8845892 - Flags: review?(hskupin) → review+
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.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/01c965ef7036
Click on Safebrowsing report button has to wait for the final URL. r=whimboo
https://hg.mozilla.org/mozilla-central/rev/01c965ef7036
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
test-only change which we want to see uplifted to aurora and beta, and maybe even esr52 later.
Flags: needinfo?(justaseem51)
Whiteboard: [lang=py] → [lang=py][checkin-needed-aurora][checkin-needed-beta]
Thank you Aseem for finishing the patch off!
https://hg.mozilla.org/releases/mozilla-aurora/rev/d2fe539270c9
Flags: in-testsuite+
Whiteboard: [lang=py][checkin-needed-aurora][checkin-needed-beta] → [lang=py][checkin-needed-beta]
https://hg.mozilla.org/releases/mozilla-beta/rev/6f8b65d518c4
Whiteboard: [lang=py][checkin-needed-beta] → [lang=py]
(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.
Whiteboard: [lang=py] → [lang=py][stockwell fixed]
(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!
You need to log in before you can comment on or make changes to this bug.