59 bytes, text/x-review-board-request
This is a follow-up from the latest discussion on bug 1353182, which is about to make the tests less fragile due to outages of remote services like support.mozilla.org. Here what needs to be done: (In reply to François Marier [:francois] from bug 1353182 comment #11) > (In reply to Henrik Skupin (:whimboo) from comment #10) > > No, but I assume there is a pref with formatting options, which is retrieved > > by Firefox and the target url in this case SUMO is build from it? We could > > easily update this pref. May this is > > `app.support.baseURL;https://support.mozilla.org/1/firefox/%VERSION%/%OS%/ > > %LOCALE%/`? If yes, we can get this switched to our local server on a new > > bug. > > Yes, it does use that pref. > > The code that opens SUMO ("Why was this page blocked?" button) is here: > https://searchfox.org/mozilla-central/rev/ > d4eaa9c2fa54d553349ac88f0c312155a4c6e89e/browser/base/content/browser.js#3195 > > and it uses this code: > > openHelpLink("phishing-malware", false, "current"); > > The openHelpLink() function is defined here: > https://searchfox.org/mozilla-central/rev/ > d4eaa9c2fa54d553349ac88f0c312155a4c6e89e/browser/base/content/utilityOverlay. > js#906 > > and uses getHelpLinkUrl(): > https://searchfox.org/mozilla-central/rev/ > d4eaa9c2fa54d553349ac88f0c312155a4c6e89e/browser/base/content/utilityOverlay. > js#901 > > which does the use app.support.baseURL pref. > > So if we can redirect all SUMO requests to a local server that always > returns a 200, we should eliminate all of the false positives. Tests we are referring to here are located in: https://dxr.mozilla.org/mozilla-central/search?q=file%3Atest_safe_browsing+path%3Atesting%2Ffirefox-ui%2F&redirect=false To test which of those tests actually need an update, run them via `mach firefox-ui-functional testing/firefox-ui/tests/functional/test_safe_browsing*` and check if support.mozilla.org is getting opened. To setup the development environment please follow those steps: https://wiki.mozilla.org/User:Mjzffr/New_Contributors
And to add... the preference `app.support.baseURL` can be changed to a locally served URL via wptserve. Therefore a new HTML page can be added by placing a file into `https://dxr.mozilla.org/mozilla-central/source/testing/firefox-ui/resources`. With `self.marionette.absolute_url(path)` the location can be retrieved, and the preference updated in `setUp()`. Something we should figure out is which parameters of the final URL are important to check. I think at least the type of page would be necessary. Francois, what else do you think would be good or a must have for you?
I want to work on this bug. Can I get more information?
(In reply to Henrik Skupin (:whimboo) from comment #1) > Something we should figure out is which parameters of the final URL are > important to check. I think at least the type of page would be necessary. > Francois, what else do you think would be good or a must have for you? I'm not sure I understand what you're asking for, but looking at the warning page test (https://dxr.mozilla.org/mozilla-central/source/testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py#96), it only seems to care that button press lead to a page loading correctly. So the dummy page could just be an empty page that returns an HTTP 200.
Flags: needinfo?(francois) → needinfo?(hskupin)
Ok, so I had a look at the URLs which are given in comment 0. And as `getHelpLinkUrl()` shows, we simply concatenate the help topic to the SUMO base URL: https://searchfox.org/mozilla-central/rev/d4eaa9c2fa54d553349ac88f0c312155a4c6e89e/browser/base/content/utilityOverlay.js#902 Which means for `phishing-malware` we end-up with: > 'https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/' + 'phishing-malware' Whereby the placeholders for version, os, and locale are getting converted to real values in `getHelpLinkURL()`. None of those three seem to be important for us, except the help topic. So I would suggest the following: 1. The safebrowsing tests set the value for `app.support.baseURL` to `self.marionette.absolute_url("sumo.html?topic=") in setUp. Which means that when clicking the button we have a URL like "support.html?topic=phishing-malware". 2. A new HTML file gets added to `testing/firefox-ui/resources/support.html`, which itself can retrieve the =`topic` parameter, and displays its value in a div with an id `topic`. 3. The test can retrieve the element via `find_element(By.ID, "topic")` and check that the correct value is set
Hi. I would like work on this bug.
Hi Blaji. You are welcome to work on this bug. Please see comment 0 in how to get started. If there are questions you can also reach me on IRC in the #automation channel. I will assign the bug to you when a first patch has been uploaded - mozreview is preferred for that. Thanks.
Comment on attachment 8865094 [details] Bug 1357372 - Restrict safebrowsing tests from reaching support.mozilla.org https://reviewboard.mozilla.org/r/136764/#review140272 It's a great start Raajit! I like the patch and from what I can see it's exactly doing what it should do. But there are some issues left which you will have to fix before you get my r+. Thank you. ::: testing/firefox-ui/resources/support.html:12 (Diff revision 1) > +</body> > +<script> > + var url = window.location.search; > + topic = url.replace("?topic=", ''); // get the 'topic' param from URL. > + > + document.getElementById('topic').innerHTML = topic; Please follow the logic as used in this test file: http://mozqa.com/data/firefox/tabbedbrowsing/openinnewtab_target.html?id=1 It's important to not inject insecure content to the HTML document. ::: testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py:28 (Diff revision 1) > 'https://www.itisatrap.org/firefox/its-an-attack.html' > ] > > self.marionette.set_pref('browser.safebrowsing.phishing.enabled', True) > self.marionette.set_pref('browser.safebrowsing.malware.enabled', True) > + self.marionette.set_pref('app.support.baseURL', self.marionette.absolute_url("support.html?topic=")) Please move this up, so we keep the alphabetical order. ::: testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py:45 (Diff revision 1) > try: > self.puppeteer.utils.permissions.remove('https://www.itisatrap.org', 'safe-browsing') > self.browser.tabbar.close_all_tabs([self.browser.tabbar.tabs]) > self.marionette.clear_pref('browser.safebrowsing.malware.enabled') > self.marionette.clear_pref('browser.safebrowsing.phishing.enabled') > + self.marionette.clear_pref('app.support.baseURL') Same here as above. ::: testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py:90 (Diff revision 1) > > button = self.marionette.find_element(By.ID, "reportButton") > button.click() > > + topic = self.marionette.find_element(By.ID, "topic") > + self.assertEquals(topic.text, "phishing-malware") You should not access page elements before the page load has been finished. See the Wait().until() call right after which ensures this. I would please this check at the end of the method. ::: testing/firefox-ui/tests/functional/security/test_safe_browsing_warning_pages.py:100 (Diff revision 1) > 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 > expected_url = self.browser.get_final_url(url) > + Please remove this addition of an empty line.
Thanks for your review and sorry I should have read the guidelines more carefully. I have a question which is unrelated to this fix, you normally assign bugs after the first patch but why not here? Anyways, I will update this with fixes ASAP.
I just missed to assign it to you. Sorry for that. It's done now.
Assignee: nobody → raajit.raj
Status: NEW → ASSIGNED
Comment on attachment 8865094 [details] Bug 1357372 - Restrict safebrowsing tests from reaching support.mozilla.org https://reviewboard.mozilla.org/r/136764/#review141004 > This line extends the 100 chars and would cause a linting failure. Please wrap the second argument to the next line. Please test with a linter. The indentation here is wrong, which will cause a failure.
Comment on attachment 8865094 [details] Bug 1357372 - Restrict safebrowsing tests from reaching support.mozilla.org https://reviewboard.mozilla.org/r/136764/#review141524 ::: commit-message-37a5b:2 (Diff revision 3) > +Bug 1357372 - Added hard coded div tag instead of dynamically creating one in support.html and wrapped the line to prevent lint failures in test file > + Please use a commit message which describes all the changes, and not what you pushed most recently. If possible even add an additional paragraph for details.
Comment on attachment 8865094 [details] Bug 1357372 - Restrict safebrowsing tests from reaching support.mozilla.org https://reviewboard.mozilla.org/r/136764/#review141004 > Please test with a linter. The indentation here is wrong, which will cause a failure. Also see https://www.python.org/dev/peps/pep-0008/#indentation
Attachment #8866830 - Attachment is obsolete: true
Sorry, I was facing some issues with hg so have to make a new review request without any previous diffs, hope thats not a problem. For the linting problem I've fixed and verified using pylint. And sorry about commit message, version control is still new to me. Thanks
Please really stop pushing new mozreview patches and marking existing ones as obsolete. This completely kills the review history, and forces me to do everything again. If you have problems like this ask on IRC and we can get this fixed for upcoming patches. Thanks.
Comment on attachment 8866887 [details] Bug 1357372 - Restrict safebrowsing tests from reaching support.mozilla.org https://reviewboard.mozilla.org/r/138496/#review142772 This looks fine. I triggered some test jobs again which all passed. I will try to land it right now. Thank you again for this patch. It will make this test way more stable.
Attachment #8866887 - Flags: review?(hskupin) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/12e5e6e8cad4 Restrict safebrowsing tests from reaching support.mozilla.org r=whimboo
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
It's a test-only change which replaces a remote URL with a locally served one for better stability. We should try to uplift it to beta and esr52.
status-firefox54: --- → affected
status-firefox-esr52: --- → affected
Whiteboard: [lang=py] → [lang=py][checkin-needed-beta][checkin-needed-esr52]
status-firefox54: affected → fixed
Whiteboard: [lang=py][checkin-needed-beta][checkin-needed-esr52] → [lang=py][checkin-needed-esr52]
status-firefox-esr52: affected → fixed
Whiteboard: [lang=py][checkin-needed-esr52] → [lang=py]
You need to log in before you can comment on or make changes to this bug.