Closed Bug 1237396 Opened 9 years ago Closed 8 years ago

Convert testSafeBrowsing_initialDownload Mozmill test to Marionette

Categories

(Testing :: Firefox UI Tests, defect)

Product:
Testing
Component:
Firefox UI Tests
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox50 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: whimboo, Assigned: bennyjr35, Mentored)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 7 obsolete files)

Bug 1237396 - Add safebrowsing test for initial download of files.
58 bytes, text/x-review-board-request
whimboo
: review+
Details
We have to convert the given test because it is very important to have a test against the real server. See bug 1175562 comment 14 ff. The mozmill test can be found here: http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/tests/remote/restartTests/testSafeBrowsing_initialDownload The firefox-ui-test should be located here: https://github.com/mozilla/firefox-ui-tests/tree/mozilla-central/firefox_ui_tests/functional/security I won't have the time to convert this test myself but I'm happy to assist and review the code.
The mozmill test is missing checks for the following files: goog-badbinurl-shavar.cache goog-badbinurl-shavar.pset goog-badbinurl-shavar.sbstore goog-unwanted-shavar.cache goog-unwanted-shavar.pset goog-unwanted-shavar.sbstore and while we're checking these, we could also do the same checks for the tracking protection lists (42 and later): mozstd-track-digest256.cache mozstd-track-digest256.pset mozstd-track-digest256.sbstore mozstd-trackwhite-digest256.cache mozstd-trackwhite-digest256.pset mozstd-trackwhite-digest256.sbstore
Francois, on bug 1237763 you said: > 2. Wait 5 minutes for the lists to finish downloading. Does that really take that long? Would our test have to wait that amount of time? Or is there a way to speed this up? I mean we only have to check for those files to be existent, right? If those are getting downloaded in parallel, we could continue earlier. But I assume that especially for bug 1237766 we would have to wait to not fail the checks due to missing list entries?
The first update should take seconds, so file presence could be checked quickly. But I'm not sure how we can guarantee the first update has the right entries? Are the lists small enough or is it guaranteed the test entries are first?
Flags: needinfo?(francois)
If it would take a long time we may have to combine all the safe browsing tests into a single test file. I don't want that we have to download the content multiple times which would add an unknown amount of time to our tests execution duration.
(In reply to Henrik Skupin (:whimboo) from comment #2) > Francois, on bug 1237763 you said: > > > 2. Wait 5 minutes for the lists to finish downloading. > > Does that really take that long? Would our test have to wait that amount of > time? Or is there a way to speed this up? If you set "browser.safebrowsing.provider.google.nextupdatetime" to 1 (i.e. a date way in the past), it should force the lists to be updated next time the browser starts up. > I mean we only have to check for > those files to be existent, right? If those are getting downloaded in > parallel, we could continue earlier. But I assume that especially for bug > 1237766 we would have to wait to not fail the checks due to missing list > entries? It's not so much a question of the time required to download the lists (they're all fairly small) but rather that there is a random delay of up to 5 minutes for the initial list download. If you change "nextupdatetime", that should force the download to take place immediately after the restart though.
Flags: needinfo?(francois)
(In reply to Gian-Carlo Pascutto [:gcp] from comment #3) > The first update should take seconds, so file presence could be checked > quickly. But I'm not sure how we can guarantee the first update has the > right entries? Are the lists small enough or is it guaranteed the test > entries are first? I'm pretty sure the initial download has all of the entries right away. At least, I've never had problems with that in my own testing. In the case of the mozmill test, it's easier though because we're only checking to see if the files are present, regardless of their content.
In that case it sounds great. Thank you for the feedback. Francois, would anyone of your team or even yourself have the time to create those Marionette tests? I'm happy to mentor who-ever will work on this bug.
Assignee: nobody → mwobensmith
(In reply to François Marier [:francois] from comment #6) > I'm pretty sure the initial download has all of the entries right away. At > least, I've never had problems with that in my own testing. Bug 1240195 and bug 1240027 have now shattered this hope I presume.
(In reply to Gian-Carlo Pascutto [:gcp] from comment #8) > (In reply to François Marier [:francois] from comment #6) > > I'm pretty sure the initial download has all of the entries right away. At > > least, I've never had problems with that in my own testing. > > Bug 1240195 and bug 1240027 have now shattered this hope I presume. Yeah, though we can still go ahead and convert the existing MozMill test since it only looks at the presence of the files on disk. The larger problem of how to test Safe Browsing properly will be tackled in bug 1240195.
Matt, how is your time availability the next weeks in regards of starting to convert this test? If you don't have time John would be interested to get started with this bug. Please let us know. Thanks.
Flags: needinfo?(mwobensmith)
If John is available now, that would be great. I'm several weeks away from freeing up time to do this. Thanks.
Flags: needinfo?(mwobensmith)
I will take this over. I have a community contributor helping out and I will be serving as a guide for him.
Assignee: mwobensmith → jdorlus
Status: NEW → ASSIGNED
Attached file Request for review (obsolete) —
Assignee: jdorlus → bennyjr169
Attached file Github PR (obsolete) —
So your latest text link only contained a sentence but not the link to the Github PR. I updated it now and also requested review from myself.
Attachment #8718559 - Attachment is obsolete: true
Attachment #8718754 - Flags: review?(hskupin)
Hi Benny. Now that I'm back from vacation I wonder if you have some updates for us on this bug? Anything you are blocked on?
Flags: needinfo?(bennyjr169)
(In reply to Henrik Skupin (:whimboo) from comment #15) > Hi Benny. Now that I'm back from vacation I wonder if you have some updates > for us on this bug? Anything you are blocked on? Hey Henrik! I hope vaca went well. I have been busy with school and such, however I hope to finish this bug this week for sure. I just need some help with the array list that you wanted me to implement.
Flags: needinfo?(bennyjr169)
I was already out when you tried to reach me on IRC yesterday. In case IRC doesn't work please let me know about your questions here instead.
Sounds good, thanks. I just need some help understanding the loop for the array list. I'll be in the IRC throughout this week.
(In reply to François Marier [:francois] from comment #1) > The mozmill test is missing checks for the following files: > > goog-badbinurl-shavar.cache > goog-badbinurl-shavar.pset > goog-badbinurl-shavar.sbstore > goog-unwanted-shavar.cache > goog-unwanted-shavar.pset > goog-unwanted-shavar.sbstore > > and while we're checking these, we could also do the same checks for the > tracking protection lists (42 and later): > > mozstd-track-digest256.cache > mozstd-track-digest256.pset > mozstd-track-digest256.sbstore > mozstd-trackwhite-digest256.cache > mozstd-trackwhite-digest256.pset > mozstd-trackwhite-digest256.sbstore Hey Francios. almost finished with this bug. I was looking at the folder that gets created and the files that download there are not these. I know there was probably an update somewhere since I stated working on this. I found a bunch of file names that I think are the ones needed, however I don't know which ones are windows specific and which are not. Some clarification would help! Thanks
Flags: needinfo?(francois)
(In reply to bennyjr169 from comment #19) > I was looking at the folder > that gets created and the files that download there are not these. I know > there was probably an update somewhere since I stated working on this. To force a list download, set these in about:config: browser.safebrowsing.provider.google.lastupdatetime = 1 browser.safebrowsing.provider.google.nextupdatetime = 1 and then restart Firefox. > I found a bunch of file names that I think are the ones needed, however > I don't know which ones are windows specific and which are not. The following should exist on all platforms: goog-badbinurl-shavar.cache goog-badbinurl-shavar.pset goog-badbinurl-shavar.sbstore goog-malware-shavar.cache goog-malware-shavar.pset goog-malware-shavar.sbstore goog-phish-shavar.cache goog-phish-shavar.pset goog-phish-shavar.sbstore goog-unwanted-shavar.cache goog-unwanted-shavar.pset goog-unwanted-shavar.sbstore mozstd-track-digest256.cache mozstd-track-digest256.pset mozstd-track-digest256.sbstore mozstd-trackwhite-digest256.cache mozstd-trackwhite-digest256.pset mozstd-trackwhite-digest256.sbstore and these ones are Windows-specific: goog-downloadwhite-digest256.cache goog-downloadwhite-digest256.pset goog-downloadwhite-digest256.sbstore
Flags: needinfo?(francois)
Francois it seems as if restarting the browser doesn't help. Running the test with the preferences set and not restarting works fine. Take a look please. https://github.com/mozilla/firefox-ui-tests/pull/328
Attached file Review Request (obsolete) —
Requesting r+ for this patch
Attachment #8722792 - Flags: review?(hskupin)
Comment on attachment 8722792 [details] Review Request There is no need to set another review request. As you can see the other attachment has it still active. I was just waiting for an update from you before I do another review.
Attachment #8722792 - Flags: review?(hskupin)
Attached file id_rsa.pub (obsolete) —
According to https://www.mozilla.org/en-US/about/governance/policies/commit/ I needed to attach a public key to the bug, however, I don't think I did it right. Please delete if it is incorrect.
Comment on attachment 8726612 [details] id_rsa.pub As of now you shouldn't need any commit level access. This would only be required if you want to push to try. But that I can do for you for the moment. Also commit level access is given to people who landed a couple of meaningful contributions and are solid with the versioning system and patch process. So I'm obsoleting the RSA key attachment now.
Attachment #8726612 - Attachment is obsolete: true
Attachment #8718754 - Attachment is obsolete: true
Attachment #8718754 - Flags: review?(hskupin)
Attachment #8722792 - Attachment is obsolete: true
Comment on attachment 8728745 [details] MozReview Request: Bug 1237396 - Add safebrowsing test for initial download of files. r?whimboo https://reviewboard.mozilla.org/r/39085/#review35829 Hey, good to see that you were able to get mozreview working from git. Reviewing code is much better now. Thanks a lot! So I checked the code and there are some items I would like to see fixed. Please check my detailed review comments. ::: testing/firefox-ui/tests/firefox_ui_tests/functional/security/manifest.ini:13 (Diff revision 1) > +[test_safe_browsingf_initial_download.py] The filename got an additional `f`. Also please sort it alphabetically. ::: testing/firefox-ui/tests/firefox_ui_tests/functional/security/test_safe_browsing_initial_download.py:1 (Diff revision 1) > +import os Please add the MPL2 license header as what we have in all the other test files. ::: testing/firefox-ui/tests/firefox_ui_tests/functional/security/test_safe_browsing_initial_download.py:10 (Diff revision 1) > + { nit: lets move this bracket a line up and put it directly behind the opening square bracket. With that we save an additional indentation of 4 spaces. ::: testing/firefox-ui/tests/firefox_ui_tests/functional/security/test_safe_browsing_initial_download.py:48 (Diff revision 1) > + 'browser.safebrowsing.downloads.remote.enabled': 'true', I do not see why we need those two preferences set given that we do not set them to false in the harness. ::: testing/firefox-ui/tests/firefox_ui_tests/functional/security/test_safe_browsing_initial_download.py:53 (Diff revision 1) > + 'privacy.trackingprotection.pbmode.enabled': 'true' This pref is also not needed. We have to take the default. ::: testing/firefox-ui/tests/firefox_ui_tests/functional/security/test_safe_browsing_initial_download.py:64 (Diff revision 1) > + self.prefs.set_pref(item, value) Preferences are still set AFTER the browser has been restarted. Francois mentioned that it should really happen before the restart. ::: testing/firefox-ui/tests/firefox_ui_tests/functional/security/test_safe_browsing_initial_download.py:66 (Diff revision 1) > + # Set Variable to tmp safebrowsing directory Lets re-phrase to "Get safebrowsing path where downloaded data gets stored" ::: testing/firefox-ui/tests/firefox_ui_tests/functional/security/test_safe_browsing_initial_download.py:71 (Diff revision 1) > + if self.platform in data['platforms']: nit: if you want you can negate this check and add `continue`. This will save an additional indentation of 4 chars for the following code. ::: testing/firefox-ui/tests/firefox_ui_tests/functional/security/test_safe_browsing_initial_download.py:73 (Diff revision 1) > + Wait(self.marionette, timeout=self.browser.timeout_page_load).until( We do not have to create an instance of Wait each time. Do it once at the beginning of the test and safe it as a variable which can be re-used here. ::: testing/firefox-ui/tests/firefox_ui_tests/functional/security/test_safe_browsing_initial_download.py:75 (Diff revision 1) > + message='Safe Browsing Files not found!') You are not waiting for files but a single file here. I would suggest that you add the item name to the message, so it is easier to spot for which file we actually failed.
Attachment #8728745 - Flags: review?(hskupin)
https://reviewboard.mozilla.org/r/39085/#review35833 Also please set a correct commit message which clearly describes what this commit contains.
Hi Benjamin, I would like to ask about the process made in the last week. Or if you are blocked by something. Please let me know. Thanks.
Flags: needinfo?(bennyjr169)
Hey Henrik, I have been a bit behind as school has started up. I will get to all of your review messages this weekend hopefully. I am also going to change my schedule a bit to allow for more time here.
Flags: needinfo?(bennyjr169)
Sounds good and no rush. :) FYI the location of the tests have been changed given that we got rid of the firefox-ui-tests package. So it all moved up one level. Make sure to pull from latest mozilla-central or inbound before continuing.
Comment on attachment 8733598 [details] MozReview Request: Bug 1237396 - Added continue to control statements. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41835/diff/1-2/
Attachment #8733598 - Attachment description: MozReview Request: Bug 1237396 - Removed r from manifest name. r=whimboo → MozReview Request: Bug 1237396 - Added wait variable to hold wait instance.
Attachment #8733598 - Attachment description: MozReview Request: Bug 1237396 - Added wait variable to hold wait instance. → MozReview Request: Bug 1237396 - Added safebrowing file name to wait message on failure.
Comment on attachment 8733598 [details] MozReview Request: Bug 1237396 - Added continue to control statements. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41835/diff/2-3/
Comment on attachment 8733598 [details] MozReview Request: Bug 1237396 - Added continue to control statements. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41835/diff/3-4/
Attachment #8733598 - Attachment description: MozReview Request: Bug 1237396 - Added safebrowing file name to wait message on failure. → MozReview Request: Bug 1237396 - Added continue to control statements.
Comment on attachment 8728745 [details] MozReview Request: Bug 1237396 - Add safebrowsing test for initial download of files. r?whimboo Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39085/diff/1-2/
Attachment #8728745 - Attachment description: MozReview Request: Bug 1237396 - Initial Commit from Github PR. r=whimboo → MozReview Request: Bug 1237396 - Added continue to control statements.
Attachment #8728745 - Flags: review?(hskupin)
Attachment #8733598 - Attachment is obsolete: true
Attachment #8733598 - Flags: review?(hskupin)
Comment on attachment 8728745 [details] MozReview Request: Bug 1237396 - Add safebrowsing test for initial download of files. r?whimboo https://reviewboard.mozilla.org/r/39085/#review38565 Code-wise this looks great now! I think we are really close to get it landed. The only thing which you didn't reply to yet is when we have to set the preferences. Please reply to that question. ::: testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py:8 (Diff revision 2) > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +import os > + > +from marionette_driver import Wait > +from firefox_puppeteer.testcases import FirefoxTestCase nit: Please switch those two lines so that we have an alphabetical order of imported modules. ::: testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py:65 (Diff revision 2) > + # Restart Browser > + self.restart() > + > + # Set Browser Preferences > + for item, value in self.browser_prefs.items(): > + self.prefs.set_pref(item, value) I still don't see any comment from you about my previously mentioned fact that setting prefs should happen before the restart. ::: testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py:73 (Diff revision 2) > + self.sb_files_path = os.path.join(self.marionette.instance.profile.profile, 'safebrowsing') > + > + def test_safe_browsing_initial_download(self): > + > + # Wait variable > + wait = Wait(self.marionette, timeout=self.browser.timeout_page_load) This looks fine! But there is no need for the comment and the extra empty line above. ::: testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py:81 (Diff revision 2) > + if self.platform not in data['platforms']: > + continue > + for item in data['files']: > + wait.until( > + lambda _: os.path.exists(os.path.join(self.sb_files_path, item)), > + message='Safe Browsing File ' + item + ' not found!') Please use a formatted string here so no concatenation needs to happen. See https://pyformat.info/ for help.
Attachment #8728745 - Flags: review?(hskupin)
Comment on attachment 8728745 [details] MozReview Request: Bug 1237396 - Add safebrowsing test for initial download of files. r?whimboo Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39085/diff/2-3/
Attachment #8728745 - Attachment description: MozReview Request: Bug 1237396 - Added continue to control statements. → MozReview Request: Bug 1237396 - Rearranged Imports.
Attachment #8728745 - Flags: review?(hskupin)
Comment on attachment 8728745 [details] MozReview Request: Bug 1237396 - Add safebrowsing test for initial download of files. r?whimboo Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39085/diff/3-4/
Attachment #8728745 - Attachment description: MozReview Request: Bug 1237396 - Rearranged Imports. → MozReview Request: Bug 1237396 - Removed comment above wait variable.
https://reviewboard.mozilla.org/r/39085/#review38565 > I still don't see any comment from you about my previously mentioned fact that setting prefs should happen before the restart. So setting the preferences before the restart doesn't work. When I do set them before the restart the safe browsing files do not download.
Comment on attachment 8728745 [details] MozReview Request: Bug 1237396 - Add safebrowsing test for initial download of files. r?whimboo Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39085/diff/4-5/
Attachment #8728745 - Attachment description: MozReview Request: Bug 1237396 - Removed comment above wait variable. → MozReview Request: Bug 1237396 - Added formatting to error message string.
Comment on attachment 8728745 [details] MozReview Request: Bug 1237396 - Add safebrowsing test for initial download of files. r?whimboo Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39085/diff/5-6/
Attachment #8728745 - Attachment description: MozReview Request: Bug 1237396 - Added formatting to error message string. → MozReview Request: Bug 1237396 - Removed '#' from preferences loop.
https://reviewboard.mozilla.org/r/39085/#review38565 > So setting the preferences before the restart doesn't work. When I do set them before the restart the safe browsing files do not download. To approve that I would like to see a final feedback from Francois here. If he thinks that this is fine we can go with it. So please ni? him for that.
Comment on attachment 8728745 [details] MozReview Request: Bug 1237396 - Add safebrowsing test for initial download of files. r?whimboo https://reviewboard.mozilla.org/r/39085/#review38709 Please make sure that you have set a correct commit message. Looks like with each submitted commit you overwrite the formerly set one. We need a clear "Bug 1237396 - description. r?whimboo" message here.
Attachment #8728745 - Flags: review?(hskupin)
Comment on attachment 8728745 [details] MozReview Request: Bug 1237396 - Add safebrowsing test for initial download of files. r?whimboo Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39085/diff/6-7/
Attachment #8728745 - Attachment description: MozReview Request: Bug 1237396 - Removed '#' from preferences loop. → MozReview Request: Bug 1237396 - Removed '#' from preferences loop. r?whimboo, francois
Attachment #8728745 - Flags: review?(hskupin)
Attachment #8728745 - Flags: review?(francois)
So for setting the preferences, it seems that if I set them before the browser is triggered to restart, the safe browsing files do not download. However, if I restart the browser and then set the preferences, the files do download. Is this okay or should I do something else?
Flags: needinfo?(francois)
Attachment #8728745 - Flags: review?(francois)
Comment on attachment 8728745 [details] MozReview Request: Bug 1237396 - Add safebrowsing test for initial download of files. r?whimboo https://reviewboard.mozilla.org/r/39085/#review38855 ::: testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py:50 (Diff revision 7) > + ] > + } > + ] > + > + browser_prefs = { > + 'browser.safebrowsing.downloads.enabled': 'true', Let's also add `'privacy.trackingprotection.enabled' : true` to make sure that the TP lists are always downloaded.
(In reply to Benjamin Forehand Jr(:bennyjr35) from comment #47) > So for setting the preferences, it seems that if I set them before the > browser is triggered to restart, the safe browsing files do not download. > However, if I restart the browser and then set the preferences, the files do > download. > > Is this okay or should I do something else? When the browser starts, it looks at the prefs and if Safe Browsing and Tracking Protection are enabled then it downloads the lists. In addition, I think that if you start the browser with these things turned off and then enable them later, that will also trigger a list download.
Flags: needinfo?(francois)
Comment on attachment 8728745 [details] MozReview Request: Bug 1237396 - Add safebrowsing test for initial download of files. r?whimboo Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39085/diff/7-8/
Attachment #8728745 - Attachment description: MozReview Request: Bug 1237396 - Removed '#' from preferences loop. r?whimboo, francois → MozReview Request: Bug 1237396 - Add safebrowsing test for initial download of files. r?whimboo
Comment on attachment 8728745 [details] MozReview Request: Bug 1237396 - Add safebrowsing test for initial download of files. r?whimboo https://reviewboard.mozilla.org/r/39085/#review38861
Attachment #8728745 - Flags: review+
Attachment #8728745 - Flags: review?(hskupin) → review+
Comment on attachment 8728745 [details] MozReview Request: Bug 1237396 - Add safebrowsing test for initial download of files. r?whimboo https://reviewboard.mozilla.org/r/39085/#review39465 Given that Francois is happy with it, I'm too! Code-wise everything looks also fine. So you have my r+! I triggered a try build to check if all is fine. If it is I will get this patch landed on mozilla-central. Thanks for all your hard work and especially wading through all the annoyences with repository changes and mozreview! I hope you still have interest in taking something else to continue learning.
Comment on attachment 8728745 [details] MozReview Request: Bug 1237396 - Add safebrowsing test for initial download of files. r?whimboo https://reviewboard.mozilla.org/r/39085/#review39475 Try push failed on linux64 for e10s and non-e10s already for the first file: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb53685ba68e&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=18746194 TEST-UNEXPECTED-ERROR | test_safe_browsing_initial_download.py TestSafeBrowsingInitialDownload.test_safe_browsing_initial_download | TimeoutException: TimeoutException: Timed out after 30.1 seconds with message: Safe Browsing File: goog-badbinurl-shavar.cache not found!
Attachment #8728745 - Flags: review+
The problem here is indeed that the preferences which we set before a call to `restart()` are all reset to their harness default values. This is most likely because Marionette itself puts those preferences in user.js, which override every preference in prefs.js during a restart of Firefox. So there is at least `enforce_gecko_prefs()` (http://marionette-client.readthedocs.org/en/latest/reference.html#marionette_driver.marionette.Marionette.enforce_gecko_prefs) which would allow us to force the preferences to be set. This would work for most of the preferences but interestingly not for 'privacy.trackingprotection.enabled'. This stays false after a restart, and as result the test will fail for each tracking protection related file. I totally dislike using the above method because it hard-kills Firefox and doesn't use the in-app restart logic. This will cause lots of problems with sessionstore and set preferences. So not sure that we want that before it got fixed. I might have to file a bug for that. Thinking more about all that I believe that the underlying problem here is with Marionette and using the user.js file. Andreas, why are we doing that? What is the need for it? Why cant we simply update the prefs.js with the values we need and let tests modify the preferences as they need to?
Flags: needinfo?(ato)
I'm not familiar with what the Marionette client is doing, I've done very little work on it. I don't think there's a good reason for why we're using user.js, except that writing a preference parser is not trivial work because of syntax inconsistencies.
Flags: needinfo?(ato)
Depends on: 1260502
Depends on: 1260504
Ok, so we can for now workaround this problem by not using our internal restart() method but the enforce_gecko_prefs() method. That only works because we do not rely on any data as written during shutdown, so this should be totally fine for now until the dependency bugs have been fixed. So that's what we need: > - self.restart() > + self.marionette.enforce_gecko_prefs(self.browser_prefs) Given that some of those prefs are not set by default to our expected value this method will trigger a restart of Firefox. The prefs will remain their value and safebrowsing and tracking protection should successfully download the files. One addition here... We definitely need a tearDown method for this test file. There we have to call the restart() method, best with the clean argument set to True, so that the next test will work with a clean profile. Otherwise our other safebrowsing tests could also fail.
Benjamin, will you be able to finish up this patch? I haven't seen an update for all the last month here. Please let me know. Thanks.
Flags: needinfo?(bennyjr169)
Hey Henrik. Yes I will be able to finish. I am running into an error with mozreview that I am seeking help on. School was quite hectic the last month so I didn't have any extra time.
Flags: needinfo?(bennyjr169)
Comment on attachment 8749032 [details] Bug 1237396 - Add safebrowsing test for initial download of files. https://reviewboard.mozilla.org/r/50705/#review48113 As discussed on IRC Benny will try to submit again, after fixing the mozreview-commit-id. On the other side this is a r- because you haven't added the tearDown method yet, which I was talking about some comments earlier.
Attachment #8749032 - Flags: review?(hskupin)
Comment on attachment 8749032 [details] Bug 1237396 - Add safebrowsing test for initial download of files. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50705/diff/1-2/
Attachment #8749032 - Flags: review?(hskupin)
Attachment #8728745 - Attachment is obsolete: true
Comment on attachment 8749032 [details] Bug 1237396 - Add safebrowsing test for initial download of files. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50705/diff/2-3/
https://reviewboard.mozilla.org/r/50705/#review48113 Figured out the issue, should be all good now!
Comment on attachment 8749032 [details] Bug 1237396 - Add safebrowsing test for initial download of files. https://reviewboard.mozilla.org/r/50705/#review49065 Good to see that we have the old commits back so that I can do an interdiff! Please see my inline comments for what still needs to be done. ::: testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py:66 (Diff revision 3) > + # Restart Browser > + self.marionette.enforce_gecko_prefs(self.browser_prefs) > + > + # Set Browser Preferences > + for item, value in self.browser_prefs.items(): > + self.prefs.set_pref(item, value) You do not have to set the preferences again. This should have already been done by `enforce_gecko_prefs()`. ::: testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py:83 (Diff revision 3) > + wait.until( > + lambda _: os.path.exists(os.path.join(self.sb_files_path, item)), > + message='Safe Browsing File: {} not found!'.format(item)) > + > + def tearDown(self): > + FirefoxTestCase.tearDown(self) This tearDown method is a no-op given that the FirefoxTestCase one would have been executed even without its addition. Please read my comment about the teardown functionality very carefully. Nothing what I wrote has actually been implemented here.
Attachment #8749032 - Flags: review?(hskupin)
Depends on: 1033450
Depends on: 1275585
Can I get some info on the moz tracking files? It seems firefox build 49.0a1 does not download the moz prefix files, whereas build 48 did. I am thinking they changed names or something, however any info would work.
Flags: needinfo?(hskupin)
(In reply to Benjamin Forehand Jr(:bennyjr35) from comment #65) > Can I get some info on the moz tracking files? It seems firefox build 49.0a1 > does not download the moz prefix files, whereas build 48 did. I am thinking > they changed names or something, however any info would work. I'm not the right person to ask. Developers of the safe browsing component will know more. So setting ni? on Francois and Gian-Carlo.
Flags: needinfo?(hskupin)
Flags: needinfo?(gpascutto)
Flags: needinfo?(francois)
On my Nightly, I have TP in private browsing enabled, and the strict list. Investigating the profile shows the blocklists were last updated 2016-04-26. Did we break the feature?
Flags: needinfo?(gpascutto)
(In reply to Benjamin Forehand Jr(:bennyjr35) from comment #65) > Can I get some info on the moz tracking files? It seems firefox build 49.0a1 > does not download the moz prefix files, whereas build 48 did. I am thinking > they changed names or something, however any info would work. If I create a fresh profile on Nightly 2016-05-25 (Linux64), I get all of the expected file: mozstd-track-digest256.cache test-forbid-simple.cache test-track-simple.cache mozstd-track-digest256.pset test-forbid-simple.pset test-track-simple.pset mozstd-track-digest256.sbstore test-forbid-simple.sbstore test-track-simple.sbstore mozstd-trackwhite-digest256.cache test-malware-simple.cache test-trackwhite-simple.cache mozstd-trackwhite-digest256.pset test-malware-simple.pset test-trackwhite-simple.pset mozstd-trackwhite-digest256.sbstore test-malware-simple.sbstore test-trackwhite-simple.sbstore test-block-simple.cache test-phish-simple.cache test-unwanted-simple.cache test-block-simple.pset test-phish-simple.pset test-unwanted-simple.pset test-block-simple.sbstore test-phish-simple.sbstore test-unwanted-simple.sbstore Do you see that as well in a fresh profile or did you change prefs? I am aware of a bug that breaks updates (bug 1274685) but you shouldn't be affected by that unless you've customized some of the urlclassifier.*Table prefs.
Flags: needinfo?(francois)
Flags: needinfo?(dlee)
(In reply to François Marier [:francois] from comment #71) > (In reply to Benjamin Forehand Jr(:bennyjr35) from comment #65) > > Can I get some info on the moz tracking files? It seems firefox build 49.0a1 > > does not download the moz prefix files, whereas build 48 did. I am thinking > > they changed names or something, however any info would work. > > If I create a fresh profile on Nightly 2016-05-25 (Linux64), I get all of > the expected file: > Ah so it's the profile. I don't know the command for mach to give it a blank profile.
The mach command always uses a fresh profile.
Can you give me the exact preferences you used Francois?
Flags: needinfo?(francois)
(In reply to Benjamin Forehand Jr(:bennyjr35) from comment #74) > Can you give me the exact preferences you used Francois? Here's all I did: 1. Create a new Firefox profile: ./firefox --no-remote --ProfileManager 2. Wait for a few seconds. 3. Look in ~/.cache/mozilla/firefox/PROFILENAME/safebrowsing to see that the mozstd-* files are there 4. Go into the preferences, under Privacy, Tracking Protection, Change Blocklist 5. Switch to the strict list 6. Restart Firefox 7. Look in ~/.cache/mozilla/firefox/PROFILENAME/safebrowsing to see that the mozfull-* files are there
Flags: needinfo?(francois)
Attachment #8758814 - Attachment is obsolete: true
Attachment #8758814 - Flags: review?(hskupin)
Comment on attachment 8749032 [details] Bug 1237396 - Add safebrowsing test for initial download of files. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50705/diff/3-4/
Attachment #8749032 - Flags: review?(hskupin)
Comment on attachment 8749032 [details] Bug 1237396 - Add safebrowsing test for initial download of files. https://reviewboard.mozilla.org/r/50705/#review53976 Thank you for the update. In general this looks fine now. But there have been made some changes you will have to revert. Please see the individual issues which I opened for you. Once that has been fixed we can trigger a try build to check if all works as expected. Thanks! ::: testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py:8 (Diff revisions 2 - 4) > > import os > > -from firefox_puppeteer.testcases import FirefoxTestCase > from marionette_driver import Wait > +from firefox_ui_harness.testcases import FirefoxTestCase That's an unrelated change. Please revert it. ::: testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py:56 (Diff revisions 2 - 4) > 'browser.safebrowsing.enabled': 'true', > 'browser.safebrowsing.malware.enabled': 'true', > 'browser.safebrowsing.provider.google.nextupdatetime': 1, > 'browser.safebrowsing.provider.mozilla.nextupdatetime': 1, > - 'privacy.trackingprotection.enabled': 'true' > + 'privacy.trackingprotection.enabled': 'true', > + 'privacy.trackingprotection.pbmode.enabled': 'true' nit: Try to always put a final comma in a list, so a diff would only show the new added line and not both. ::: testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py:69 (Diff revisions 2 - 4) > > # Get safebrowsing path where downloaded data gets stored > self.sb_files_path = os.path.join(self.marionette.instance.profile.profile, 'safebrowsing') > > + def tearDown(self): > + self.restart(clean=True) Put that into a try/except as what we have for all the other tests which call the parent tearDown method. That will ensure that the testcase tearDown will always be executed. ::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:69 (Diff revision 4) > > # Also close all remaining tabs > self.browser.tabbar.close_all_tabs([self.browser.tabbar.tabs[0]]) > self.browser.tabbar.tabs[0].switch_to() > > - def restart(self, flags=None): > + def restart(self, flags=None, **kwargs): I just noticed that flags isn't used at all right now. So lets get this removed. ::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:76 (Diff revision 4) > > :param flags: Specific restart flags for Firefox > """ > + > + # Marionette doesn't keep the former context, so restore to chrome > + self.marionette.set_context('chrome') Please do not move this line. Now we are in content scope but not chrome scope by default after an in_app triggered restart. ::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:86 (Diff revision 4) > - Components.utils.import("resource://gre/modules/Services.jsm"); > + Components.utils.import("resource://gre/modules/Services.jsm"); > - let cancelQuit = Components.classes["@mozilla.org/supports-PRBool;1"] > + let cancelQuit = Components.classes["@mozilla.org/supports-PRBool;1"] > .createInstance(Components.interfaces.nsISupportsPRBool); > - Services.obs.notifyObservers(cancelQuit, "quit-application-requested", null); > + Services.obs.notifyObservers(cancelQuit, "quit-application-requested", null); > - """) > + """) > - self.marionette.restart(in_app=True) > + self.marionette.restart(**kwargs) By default we should always use `in_app=True`. Anything else is not reliable. So you can define it in the function header appropriately.
Attachment #8749032 - Flags: review?(hskupin)
https://reviewboard.mozilla.org/r/50705/#review53976 > That's an unrelated change. Please revert it. If I use the old import method I get an import error: ImportError: cannot import name FirefoxTestCase. This is testing against moz inbound. > Please do not move this line. Now we are in content scope but not chrome scope by default after an in_app triggered restart. The reason I moved this was because doing some reading I found out that Components.utils.import() needs a context. If I don't set a context, Components.utils() becomes undefined and self.marionette.execute_script throws a Javascript error. Traceback (most recent call last): File "/home/bennyjr/mozilla/mozilla-central-1/mozilla-inbound/testing/marionette/harness/marionette/marionette_test.py", line 374, in run self.tearDown() File "/home/bennyjr/mozilla/mozilla-central-1/mozilla-inbound/testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py", line 70, in tearDown self.restart(clean=True) File "/home/bennyjr/mozilla/mozilla-central-1/mozilla-inbound/testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py", line 84, in restart """) File "/home/bennyjr/mozilla/mozilla-central-1/mozilla-inbound/testing/marionette/client/marionette_driver/marionette.py", line 1572, in execute_script rv = self._send_message("executeScript", body, key="value") File "/home/bennyjr/mozilla/mozilla-central-1/mozilla-inbound/testing/marionette/client/marionette_driver/decorators.py", line 36, in _ return func(*args, **kwargs) File "/home/bennyjr/mozilla/mozilla-central-1/mozilla-inbound/testing/marionette/client/marionette_driver/marionette.py", line 688, in _send_message self._handle_error(err) File "/home/bennyjr/mozilla/mozilla-central-1/mozilla-inbound/testing/marionette/client/marionette_driver/marionette.py", line 721, in _handle_error raise errors.lookup(error)(message, stacktrace=stacktrace) ```sh JavascriptException: JavascriptException: TypeError: Components.utils is undefined stacktrace: execute_script @base.py, line 84 inline javascript, line 1 src: " Components.utils.import("resource://gre/modules/Services.jsm");" Stack: @base.py:1:13 @base.py:0:49 ``` > By default we should always use `in_app=True`. Anything else is not reliable. So you can define it in the function header appropriately. Setting in_app to true conflicts with the restart method algorithm within marionette itself. See this: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette_driver/marionette.py#1101
https://reviewboard.mozilla.org/r/50705/#review53976 > If I use the old import method I get an import error: ImportError: cannot import name FirefoxTestCase. > > This is testing against moz inbound. Ah, right. I didn't notice the change in the first part of the line. So just switch the order of the lines so that it is as before. > The reason I moved this was because doing some reading I found out that Components.utils.import() needs a context. If I don't set a context, Components.utils() becomes undefined and self.marionette.execute_script throws a Javascript error. > > Traceback (most recent call last): > File "/home/bennyjr/mozilla/mozilla-central-1/mozilla-inbound/testing/marionette/harness/marionette/marionette_test.py", line 374, in run > self.tearDown() > File "/home/bennyjr/mozilla/mozilla-central-1/mozilla-inbound/testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py", line 70, in tearDown > self.restart(clean=True) > File "/home/bennyjr/mozilla/mozilla-central-1/mozilla-inbound/testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py", line 84, in restart > """) > File "/home/bennyjr/mozilla/mozilla-central-1/mozilla-inbound/testing/marionette/client/marionette_driver/marionette.py", line 1572, in execute_script > rv = self._send_message("executeScript", body, key="value") > File "/home/bennyjr/mozilla/mozilla-central-1/mozilla-inbound/testing/marionette/client/marionette_driver/decorators.py", line 36, in _ > return func(*args, **kwargs) > File "/home/bennyjr/mozilla/mozilla-central-1/mozilla-inbound/testing/marionette/client/marionette_driver/marionette.py", line 688, in _send_message > self._handle_error(err) > File "/home/bennyjr/mozilla/mozilla-central-1/mozilla-inbound/testing/marionette/client/marionette_driver/marionette.py", line 721, in _handle_error > raise errors.lookup(error)(message, stacktrace=stacktrace) > > ```sh > JavascriptException: JavascriptException: TypeError: Components.utils is undefined > stacktrace: > execute_script @base.py, line 84 > inline javascript, line 1 > src: " Components.utils.import("resource://gre/modules/Services.jsm");" > Stack: > @base.py:1:13 > @base.py:0:49 > ``` In that case use `with self.marionette.using_context('chrome')` for the call to `execute_script()` > Setting in_app to true conflicts with the restart method algorithm within marionette itself. See this: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette_driver/marionette.py#1101 Why does it conflict? In our tests we always want it to be in_app because otherwise the browser process will be killed by marionette harness and it will result in dataloss (e.g. sessionstore).
https://reviewboard.mozilla.org/r/50705/#review53976 > Why does it conflict? In our tests we always want it to be in_app because otherwise the browser process will be killed by marionette harness and it will result in dataloss (e.g. sessionstore). Well according to the restart method within marionette, if in_app and clean are set to true, it raises a ValueError. See this line: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette_driver/marionette.py#1117
https://reviewboard.mozilla.org/r/50705/#review53976 > Well according to the restart method within marionette, if in_app and clean are set to true, it raises a ValueError. > > See this line: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette_driver/marionette.py#1117 The default of `True` should be part of the method definition. But as you noticed here, the call to `restart()` has to use `in_app=False`, right.
Comment on attachment 8749032 [details] Bug 1237396 - Add safebrowsing test for initial download of files. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50705/diff/4-5/
Attachment #8749032 - Attachment description: MozReview Request: Bug 1237396 - Add safebrowsing test for initial download of files. r?whimboo → Bug 1237396 - Add safebrowsing test for initial download of files.
Attachment #8749032 - Flags: review?(hskupin)
Comment on attachment 8749032 [details] Bug 1237396 - Add safebrowsing test for initial download of files. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50705/diff/5-6/
Attachment #8749032 - Flags: review?(hskupin)
Comment on attachment 8749032 [details] Bug 1237396 - Add safebrowsing test for initial download of files. https://reviewboard.mozilla.org/r/50705/#review54308 This looks to be kinda close to get landed! Thanks for the update Benjamin. As noted below there is only one more thing for a cleaner API. If you can get this done that would be great. ::: testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py:70 (Diff revisions 4 - 6) > # Get safebrowsing path where downloaded data gets stored > self.sb_files_path = os.path.join(self.marionette.instance.profile.profile, 'safebrowsing') > > def tearDown(self): > - self.restart(clean=True) > + try: > + self.restart(in_app=False, clean=True) Just a nit. I would find it more obvious if we would force the `in_app=False` in case of `clean=True` inside the testcase.restart() method. So inside a test you will only have to specify the clean parameter.
I triggered a try build to check if the patch is doing what it is expected to do. Results will be visible here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b807baebd66 Have you tested your patch also on Windows? If not please do so and attach a log to this bug. Thanks.
Comment on attachment 8749032 [details] Bug 1237396 - Add safebrowsing test for initial download of files. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50705/diff/6-7/
Attachment #8749032 - Flags: review?(hskupin)
So RyanVM helped me get the mozilla-build package working so I have a log. --- 0:00.00 LOG: MainThread INFO Using workspace for temporary data: "c:\Users\bfor ehand\mozilla-inbound" 0:05.51 LOG: MainThread INFO Initial Profile Destination is "c:\users\bforeh~1\ appdata\local\temp\tmph46zii.mozrunner" 0:05.53 LOG: MainThread INFO starting httpd 0:05.53 LOG: MainThread INFO running httpd on http://127.0.0.1:49381/ 0:05.53 LOG: MainThread INFO running with e10s: True 0:05.54 LOG: MainThread mozversion INFO application_buildid: 20160621111345 0:05.54 LOG: MainThread mozversion INFO application_changeset: bd393975aadd0769 b1d67b595470d7b002fc15f4 0:05.54 LOG: MainThread mozversion INFO application_display_name: Nightly 0:05.54 LOG: MainThread mozversion INFO application_id: {ec8030f7-c20a-464f-9b0 e-13a3a9e97384} 0:05.54 LOG: MainThread mozversion INFO application_name: Firefox 0:05.54 LOG: MainThread mozversion INFO application_remotingname: firefox 0:05.54 LOG: MainThread mozversion INFO application_vendor: Mozilla 0:05.54 LOG: MainThread mozversion INFO application_version: 49.0a1 0:05.54 LOG: MainThread mozversion INFO platform_buildid: 20160621111345 0:05.54 LOG: MainThread mozversion INFO platform_changeset: bd393975aadd0769b1d 67b595470d7b002fc15f4 0:05.54 LOG: MainThread mozversion INFO platform_version: 49.0a1 0:05.54 SUITE_START: MainThread 1 0:05.56 TEST_START: MainThread test_safe_browsing_initial_download.py TestSafeB rowsingInitialDownload.test_safe_browsing_initial_download 0:23.53 TEST_END: MainThread PASS 0:23.53 LOG: MainThread INFO SUMMARY ------- 0:23.53 LOG: MainThread INFO passed: 1 0:23.53 LOG: MainThread INFO failed: 0 0:23.53 LOG: MainThread INFO todo: 0 0:23.64 LOG: MainThread INFO mode: e10s 0:23.64 SUITE_END: MainThread Summary ======= Ran 1 tests Expected results: 1 Unexpected results: 0 OK --- Hopefully this helps!
Attachment #8749032 - Flags: review?(hskupin) → review+
Comment on attachment 8749032 [details] Bug 1237396 - Add safebrowsing test for initial download of files. https://reviewboard.mozilla.org/r/50705/#review57238 The next revision is it! Please do the change and we can get your patch landed. Thanks a lot for also testing on Windows - I assume you also did a full testrun to ensure we do not break any following tests. ::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:84 (Diff revisions 5 - 7) > Components.utils.import("resource://gre/modules/Services.jsm"); > let cancelQuit = Components.classes["@mozilla.org/supports-PRBool;1"] > .createInstance(Components.interfaces.nsISupportsPRBool); > Services.obs.notifyObservers(cancelQuit, "quit-application-requested", null); > """) > - if 'clean' in kwargs: > + if 'clean' in kwargs is True: A better way doing that is clearly the following: `if kwargs.get('clean')`. It will check if clean is present and return its value. If not it will return None instead which would resolve as False for the if check.
Comment on attachment 8749032 [details] Bug 1237396 - Add safebrowsing test for initial download of files. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50705/diff/7-8/
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/436cebf9fd49 Add safebrowsing test for initial download of files. r=whimboo
https://hg.mozilla.org/mozilla-central/rev/436cebf9fd49
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
See Also: → 1282056
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: