967568 - Create Mozmill test for Firefox's safebrowsing feature

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

There are a couple of features that require Safebrowsing in FF (phishing and malware detection on the location bar, and application reputation). We need automated testing for these with the production safebrowsing servers, to make sure that updates and parsing are working correctly.

Using xpcshell tests with mock safebrowsing servers is not a great option, since it won't catch changes on the Google side (for example, when they changed the name of the apikey parameter without notifying anyone, or we implement an undocumented protocol like Application Reputation, or if our apikey gets throttled for any reason).

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

Matt, is this something that your team works on?

Comment 2

[Matt Wobensmith [:mwobensmith][:matt:]]

Our team could certainly take this on. Happy to do it.

I could see this being run as a series of manual tests, to be performed during certification passes. You mention, though, that you'd like it to be automated. We could do both.

If there are links to any specs on features that touch safe browsing - like aforementioned phishing/malware detection/application reputation - it would help us a lot in assessing the work and putting together test plans.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Part of the protocol is documented here: https://developers.google.com/safe-browsing/developers_guide_v2?csw=1

Unfortunately, the protocol has evolved and that page wasn't updated:
https://bugzilla.mozilla.org/show_bug.cgi?id=806422
https://bugzilla.mozilla.org/show_bug.cgi?id=783047
https://bugzilla.mozilla.org/show_bug.cgi?id=957091

The Application Reputation part is entirely undocumented as Monica is reverse-engineering it from Chrome sources. I might force her to write some documentation before allowing her to land the patches, though :)

That said, one thing the original comment points out is that we want to catch undocumented changes, so we really need some sort of a automated, functional test that hits the real network.

Comment 4

[[:mmc] Monica Chew (no longer reading bugmail)]

+1 on the need for automated tests, not manual adhoc ones. There is unfortunately no "certification pass" when the server side can change at will :)

Comment 5

[Matt Wobensmith [:mwobensmith][:matt:]]

Thanks Monica. Ignore my comments about manual tests and cert pass. This is me ruminating on general QA needs and not a part of the work here. 

I'll work with you to help develop the automated test(s) needed to cover this API. In the meantime, if you have any ideas of how the tests should work or what they should cover, add them here. I'll also sync up with you in other modes of communication shortly.

Comment 6

[[:mmc] Monica Chew (no longer reading bugmail)]

Per discussion with whimboo, moving component. Safebrowsing relies on lists that we download externally from Google. We need a mozmill test that can make external network connections on release builds to make sure that we don't break list updates.

The steps to test are similar to https://wiki.mozilla.org/Releases/Firefox_28/Test_Plan#Staging:

1) Start up Firefox with a clean profile
2) Change pref urlclassifier.max-complete-age to 300 seconds, or whatever value is acceptable to QA team
3) Wait that length of time
4) Look for goog-phish-shavar, goog-malware-shavar, goog-badbinurl-shavar, and (on windows only) goog-downloadwhite-digest256 in safebrowsing directory of the non-roaming profile. On Mac that profile directory is ~/Library/Caches/Firefox/Profiles/<profile_name>/safebrowsing. On Linux, it's in ~/.cache/mozilla/firefox/<profile_name>/safebrowsing.

Comment 7

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

Monica, what would be the minimum amount of time we should wait here in case of max-complete-age? I'm concerned that we could miss important data, if we only wait e.g. 30s. Reason I'm asking is that our tests should not stall too long in waiting for something. Also is there an event we might be able to wait for? Also could we trigger the download of those files directly via the Safebrowsing API? Not sure when it will be started otherwise.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Fwiw, the delays for updating aren't controlled by max-complete-age. That one determines how long a full hash completion is considered valid before the remote completer is probed again (okay - that probably doesn't make sense to anyone but mmc!).

The relevant settings/values are these, not all of them are exposed via prefs right now:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/content/listmanager.js#36
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/content/listmanager.js#196
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/content/listmanager.js#199
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/content/listmanager.js#246
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/content/listmanager.js#397

Monica, for mozmill I think we have to (only) expose the two "initialUpdateDelay" related ones.

Comment 9

[[:mmc] Monica Chew (no longer reading bugmail)]

Re: triggering downloads with the safebrowsing API directly, it would be a more convincing test if we didn't diverge from the actual production code. There is already a lot of test-only safebrowsing code that reduces my confidence in existing tests.

To force updates, we could call https://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsIUrlClassifierStreamUpdater.idl#35 and wait for a success callback.

However, that would mean constructing a lot of things that might break in prod without our noticing, if we force updates in test.

We can also consider refactoring to make things easier to test. gcp, what do you think?

Comment 10

[Gian-Carlo Pascutto [:gcp]]

I would refactor to make the initial update delay configurable (see prev comment) and set it in the tests, forcing it to happen after 1 minute, and do the check whether the safebrowsing files have appeared at 2 minutes.

What would be really really nice is if there's some known URL that's guaranteed to be in the very first update you receive, because then we could actually do more testing if the feature works (rather than just updating). But I think that's not the case? Forcing smaller update delays (say, 1 minute instead of 15-45) and then waiting, say, 10 cycles (=10 minutes), may allow for some tests that require the entire database. But I don't know if a 10 minute test is at all reasonable, nor whether that won't get our test client throttled on Googles' side. So I don't think we can do this right now.

Comment 11

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

Being able to configure the initial delay would be fantastic! Lets do that. Gian-Carlo, would you work on that? 

Regarding additional tests, what about retrieving one of the URLs sent with the updates and using that specific site for the wanted tests? I assume there is an API to access the data from our local storage.

Comment 12

[Gian-Carlo Pascutto [:gcp]]

>what about retrieving one of the URLs sent with the updates and using that specific site for the wanted tests?

We don't know the URLs in the database - the database consists of truncated SHA-256 hashes of the URLs.

Comment 13

[[:mmc] Monica Chew (no longer reading bugmail)]

This bit us again: bug 997759

I filed bug 997872 for making the updates configurable. Henrik, is that all you would need from us?

Comment 14

[Gian-Carlo Pascutto [:gcp]]

I just fixed bug 997759. The delay is not configurable - it turns out it was supposed to be no more than 3 seconds for the very first update on a clean profile, but it was longer due to a bug (up to 5 mins). I fixed that bug, so now a fresh profile will start the first update after 3 seconds. That's probably short enough for this!

Comment 15

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

Anthony, could Mihaela help us to get this test implemented? I would appreciate that.

Comment 16

[Mihaela Velimiroviciu (:mihaelav)]

So, based on comment 6 and the subsequent ones, are these the correct steps to test?

1) Start up Firefox with a clean profile
2) Wait 3s
3) Look for goog-phish-shavar, goog-malware-shavar, goog-badbinurl-shavar, and (on windows only) goog-downloadwhite-digest256 in safebrowsing directory of the non-roaming profile. On Mac that profile directory is ~/Library/Caches/Firefox/Profiles/<profile_name>/safebrowsing. On Linux, it's in ~/.cache/mozilla/firefox/<profile_name>/safebrowsing.

Comment 17

[[:mmc] Monica Chew (no longer reading bugmail)]

Yes, thank you Mihaela!

Comment 18

[Gian-Carlo Pascutto [:gcp]]

To clarify: the delay is 3 seconds after SafeBrowsing has started up. SafeBrowsing itself won't start up until about ~2s after the browser is launched. So you need to delay *at least*, say, 6 seconds, before checking.

Comment 19

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

Gian-Carlo, are there observer notifications we could make use of here? I don't like that we have to wait, instead I would like to see a hard timeout set for waitFor() to e.g. 30s, but it would return as soon as an notification appears.

Comment 20

[Gian-Carlo Pascutto [:gcp]]

>Gian-Carlo, are there observer notifications we could make use of here?

No.

Comment 21

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: safebrowsing test

Comment 22

[Andrei Eftimie]

Comment on attachment 8414315 [details] [diff] [review]
safebrowsing test

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

::: firefox/tests/functional/testSecurity/testSafeBrowsing.js
@@ +14,5 @@
> + * Check if browser correctly downloads the safebrowsing files
> + */
> +function testsafeBrowsing() {
> +  // wait for 10 seconds for safebrowsing to start
> +  controller.sleep(10000);

Do we really have to wait 10 seconds?
From the comments in the bug this should start at 3. I agree we should leave some padding, but 7 is pretty high.
I've gotten good results with 6. (I've seen intermittent failures with 4-5s).


Since there is no event to wait for, I wonder if we can wait for one of these files to appear in the profile directory.

@@ +21,5 @@
> +  assert.ok(profile.exists())
> +
> +  var goog_phish_shavar_cache = profile.clone();
> +  goog_phish_shavar_cache.appendRelativePath("safebrowsing");
> +  goog_phish_shavar_cache.appendRelativePath("goog-phish-shavar.cache");

You can merge both lines into one call like:
> appendRelativePath("safebrowsing/goog-phish-shavar.cache")

@@ +49,5 @@
> +  goog_malware_shavar_sbstore.appendRelativePath("safebrowsing");
> +  goog_malware_shavar_sbstore.appendRelativePath("goog-malware-shavar.sbstore");
> +  assert.ok(goog_malware_shavar_sbstore.exists());
> +
> +

nit: extra line here

Comment 23

[Gian-Carlo Pascutto [:gcp]]

>From the comments in the bug this should start at 3.

At least 5s (2s startup, 3s delay), see comment 18, and you need some time to actually download the files which is hard to predict as it depends on network conditions and the third party provider. (I'm surprised the test can pass with 4s, though it probably depends on how the time measurement is linked to the browser startup)

>Since there is no event to wait for, I wonder if we can wait for one of these files to appear in the profile directory.

This seems by far the best option. There's no point in waiting longer if the expected files have appeared already.

I wonder if it's worthwhile to add a check that the downloadwhite* files do not appear on non-Windows platforms? Though in that case, you do need to put a minimal delay before checking if they exist so it would break the above optimization again. 

So maybe wait 6s, check for files that should not be there (on not-Windows), and then check every second for at most 15s or so for the others to appear (all platforms). Sounds reasonable?

Comment 24

[Mihaela Velimiroviciu (:mihaelav)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #23)
> >Since there is no event to wait for, I wonder if we can wait for one of these files to appear in the profile directory.
> 
> This seems by far the best option. There's no point in waiting longer if the
> expected files have appeared already.
Is there any order in which these files are downloaded? It would be good to know (if possible) which is the last one to appear so I can wait for it...

Comment 25

[Gian-Carlo Pascutto [:gcp]]

Due to the way how the updates work, they'll all appear simultaneously. But that's just an implementation detail, so depending on that or their order seems pretty bad to me.

Comment 26

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: updated test

Comment 27

[Andrei Eftimie]

Comment on attachment 8414540 [details] [diff] [review]
updated test

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

We are going to have a new testrun defined (see https://github.com/mozilla/mozmill-automation/issues/147)
This test is going to be the first of the "structural" testrun.
Please update the location of the test, it should probably be:
> firefox/tests/structural/testSecurity/testSafeBrowsing.js
Also include all relevant manifest files.

::: firefox/tests/functional/testSecurity/testSafeBrowsing.js
@@ +21,5 @@
> +  assert.ok(profile.exists())
> +
> +  files.goog_phish_shavar_cache = profile.clone();
> +  files.goog_phish_shavar_cache.appendRelativePath("safebrowsing");
> +  files.goog_phish_shavar_cache.appendRelativePath("goog-phish-shavar.cache");

To ease this you could create an array with all files like:
> files = [
>   "goog-phish-shavar.pset",
>   "goog-phish-shavar.sbstore"
> ]

And then iterate over it, clone the profile, append the paths, and check them.

@@ +55,5 @@
> +  files.goog_badbinurl_shavar_sbstore = profile.clone();
> +  files.goog_badbinurl_shavar_sbstore.appendRelativePath("safebrowsing");
> +  files.goog_badbinurl_shavar_sbstore.appendRelativePath("goog-badbinurl-shavar.sbstore");
> +
> +  for(let file in files) {

nit: missing space

@@ +56,5 @@
> +  files.goog_badbinurl_shavar_sbstore.appendRelativePath("safebrowsing");
> +  files.goog_badbinurl_shavar_sbstore.appendRelativePath("goog-badbinurl-shavar.sbstore");
> +
> +  for(let file in files) {
> +    controller.waitFor(function () {

Use the fat arrow syntax for anonymous functions.

@@ +58,5 @@
> +
> +  for(let file in files) {
> +    controller.waitFor(function () {
> +      return files[file].exists();
> +    }, files[file].path + " exists", 10000);

Use a constant to mark the timeout of 10s.

@@ +74,5 @@
> +  winFiles.goog_downloadwhite_digest256_sbstore.appendRelativePath("safebrowsing");
> +  winFiles.goog_downloadwhite_digest256_sbstore.appendRelativePath("goog-downloadwhite-digest256.sbstore");
> +
> +  if (mozmill.isWindows) {
> +    for(let file in winFiles) {

nit: missing space

@@ +86,5 @@
> +    var filesNotExist = true;
> +
> +    try {
> +      // wait maximum 15 seconds for windows specific files to appear
> +      controller.waitFor(() => {

This should be assert.waitFor

And you don't need a try/catch, the waitFor error message should suffice.

Comment 28

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: updated patch

Comment 29

[Andrei Eftimie]

Comment on attachment 8417849 [details] [diff] [review]
updated patch

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

It seems you have overcomplicated yourself with several loops.
The code can get much tidier, see inline.

Please also add a manifest.ini file in the structural/ folder.

::: firefox/tests/structural/testSecurity/testSafeBrowsing.js
@@ +17,5 @@
> +  "goog-badbinurl-shavar.pset",
> +  "goog-badbinurl-shavar.sbstore"
> +];
> +const WIN_FILE_NAMES = [
> +    "goog-downloadwhite-digest256.cache",

nit: indentation

@@ +34,5 @@
> +  var profile = Services.dirsvc.get("ProfD", Ci.nsIFile);
> +  assert.ok(profile.exists());
> +
> +  var files = {};
> +  for (let file in FILE_NAMES) {

This might be even better it it used forEach on the FILE_NAMES array.

@@ +35,5 @@
> +  assert.ok(profile.exists());
> +
> +  var files = {};
> +  for (let file in FILE_NAMES) {
> +    files.goog_phish_shavar_cache = profile.clone();

I'm not sure what you are doing here. You don't need to save the cloned profile into a big "files" object.
Just assign profile.clone to a local variable, append the paths then wait for the file to exist, all in a single loop.

@@ +40,5 @@
> +    files.goog_phish_shavar_cache.appendRelativePath("safebrowsing");
> +    files.goog_phish_shavar_cache.appendRelativePath(FILE_NAMES[file]);
> +  }
> +
> +  for (let file in files) {

As said above, have a single loop

@@ +56,5 @@
> +
> +  if (mozmill.isWindows) {
> +    for (let file in winFiles) {
> +      controller.waitFor(() => {
> +        return winFiles[file].exists();

This is also broken, you only check the last file here, you always overwrite the same property in the code above.

@@ +64,5 @@
> +  else {
> +    // windows specific files should not appear
> +    var filesNotExist = true;
> +
> +    try {

Instead of a try/catch and because you _want_ to wait 15 seconds, you might be better of using a regular sleep(1500) and use assert.ok against !file.exists()

@@ +66,5 @@
> +    var filesNotExist = true;
> +
> +    try {
> +      // wait maximum 15 seconds for windows specific files to appear
> +      controller.waitFor(() => {

waitFor is a method of the Assertion module. This should be `assert.waitFor`. 
While controller does also expose waitFor directly, this is just a shortcut, and might be deprecated in future versions.

Please update all controller.waitFor method calls.

@@ +68,5 @@
> +    try {
> +      // wait maximum 15 seconds for windows specific files to appear
> +      controller.waitFor(() => {
> +        for (let file in winFiles) {
> +          filesNotExist = !winFiles[file].exists();

You can remove both negations from here and from the return value.

Comment 30

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: patch

Comment 31

[Andrei Eftimie]

Comment on attachment 8417924 [details] [diff] [review]
patch

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

I like the new changes.
Since we're already going to wait on non-windows platforms for at least 6 seconds, I think a sleep is the best solution.


*You forgot the other manifest file again.

::: firefox/tests/structural/testSecurity/testSafeBrowsing.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +Cu.import("resource://gre/modules/Services.jsm");

nit: leave an empty line here

@@ +42,5 @@
> +  });
> +
> +  // wait for the Windows specific files to appear
> +  if (!mozmill.isWindows) {
> +    controller.sleep(1500);

Comment 23 says the waiting time 6 seconds. If the windows specific files are not present by that time on non-windows machines we should be good.

Comment 32

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: patch

Added the manifest file and updated the sleep time

Comment 33

[Andrei Eftimie]

Comment on attachment 8417998 [details] [diff] [review]
patch

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

Looks good to me.

Comment 34

[[:mmc] Monica Chew (no longer reading bugmail)]

Dear Mihaela and Andrei,

Thank you so much for working on this bug. It is a huge relief to know that there's an automated test somewhere that can check for regressions with these updates. A test like this would have saved us a lot of grief in the past -- thank you so much for saving us future grief! :)

Regards,
Monica

Comment 35

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

Comment on attachment 8417998 [details] [diff] [review]
patch

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

::: firefox/tests/structural/testSecurity/manifest.ini
@@ +1,1 @@
> +[testSafeBrowsing.js]

This is too general. If we have to add more tests, it wont tell you what it contains. Maybe testSafeBrowsing_initialDownload.js?

::: firefox/tests/structural/testSecurity/testSafeBrowsing.js
@@ +18,5 @@
> +  { name: "goog-badbinurl-shavar.pset" },
> +  { name: "goog-badbinurl-shavar.sbstore" },
> +  { name: "goog-downloadwhite-digest256.cache", win: true },
> +  { name: "goog-downloadwhite-digest256.pset", win: true },
> +  { name: "goog-downloadwhite-digest256.sbstore", win: true }

Maybe calling the property `windowsOnly` to better understand what it is about?

@@ +22,5 @@
> +  { name: "goog-downloadwhite-digest256.sbstore", win: true }
> +];
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();

This test needs to be a restart test, and has to ensure that a new profile is being used. Otherwise formerly run tests will interfere.

@@ +30,5 @@
> + * Check if browser correctly downloads the safebrowsing files
> + */
> +function testSafeBrowsing() {
> +  var profile = Services.dirsvc.get("ProfD", Ci.nsIFile);
> +  assert.ok(profile.exists());

I don't see why this has to be in the test. It's setup logic. Also no need to assert for the profile. That will always be present.

@@ +39,5 @@
> +    file.appendRelativePath("safebrowsing");
> +    file.appendRelativePath(aName.name);
> +
> +    files.push({ file: file, name: aName });
> +  });

Same here. All setup logic.

@@ +44,5 @@
> +
> +  // wait for the Windows specific files to appear
> +  if (!mozmill.isWindows) {
> +    controller.sleep(6000);
> +  }

You do not want to use sleep here. Instead use the waitFor() from below directly for each of the files we expect to appear with a maximum timeout of TIMEOUT. Except those which are Windows only. For the latter we might have to add a sleep for sure, but this one can be much shorter.

Comment 36

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: updated

(In reply to Henrik Skupin (:whimboo) from comment #35)
> Comment on attachment 8417998 [details] [diff] [review]
> patch
> 
> Review of attachment 8417998 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/tests/structural/testSecurity/manifest.ini
> @@ +1,1 @@
> > +[testSafeBrowsing.js]
> 
> This is too general. If we have to add more tests, it wont tell you what it
> contains. Maybe testSafeBrowsing_initialDownload.js?

Done

> ::: firefox/tests/structural/testSecurity/testSafeBrowsing.js
> @@ +18,5 @@
> > +  { name: "goog-badbinurl-shavar.pset" },
> > +  { name: "goog-badbinurl-shavar.sbstore" },
> > +  { name: "goog-downloadwhite-digest256.cache", win: true },
> > +  { name: "goog-downloadwhite-digest256.pset", win: true },
> > +  { name: "goog-downloadwhite-digest256.sbstore", win: true }
> 
> Maybe calling the property `windowsOnly` to better understand what it is
> about?

Done
 
> @@ +22,5 @@
> > +  { name: "goog-downloadwhite-digest256.sbstore", win: true }
> > +];
> > +
> > +function setupModule(aModule) {
> > +  aModule.controller = mozmill.getBrowserController();
> 
> This test needs to be a restart test, and has to ensure that a new profile
> is being used. Otherwise formerly run tests will interfere.

Done: made the test restartless and move dit to the restart tests directory

> @@ +30,5 @@
> > + * Check if browser correctly downloads the safebrowsing files
> > + */
> > +function testSafeBrowsing() {
> > +  var profile = Services.dirsvc.get("ProfD", Ci.nsIFile);
> > +  assert.ok(profile.exists());
> 
> I don't see why this has to be in the test. It's setup logic. Also no need
> to assert for the profile. That will always be present.

Done: moved the code to the setupModule
 
> @@ +39,5 @@
> > +    file.appendRelativePath("safebrowsing");
> > +    file.appendRelativePath(aName.name);
> > +
> > +    files.push({ file: file, name: aName });
> > +  });
> 
> Same here. All setup logic.

Done: moved to setupModule

> @@ +44,5 @@
> > +
> > +  // wait for the Windows specific files to appear
> > +  if (!mozmill.isWindows) {
> > +    controller.sleep(6000);
> > +  }
> 
> You do not want to use sleep here. Instead use the waitFor() from below
> directly for each of the files we expect to appear with a maximum timeout of
> TIMEOUT. Except those which are Windows only. For the latter we might have
> to add a sleep for sure, but this one can be much shorter.

This sleep time of 6 second was suggested in comment #23 and it only applies when we are on non-Windows OS, to give the window specific files enough time to appear in case of a related bug. The code would get more complicated with this sleep moved somewhere later and the result would be the same (same sleep time) in the end.

Comment 37

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

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #36)
> This sleep time of 6 second was suggested in comment #23 and it only applies
> when we are on non-Windows OS, to give the window specific files enough time
> to appear in case of a related bug. The code would get more complicated with
> this sleep moved somewhere later and the result would be the same (same
> sleep time) in the end.

No, it wont. We already wait for all the other files to appear. I don't want to loose that time by simply waiting that early. Also your if condition is wrong, as I have seen now. On Linux and Mac you will wait 6s but not on Windows.

Comment 38

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #37)
> (In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #36)
> > This sleep time of 6 second was suggested in comment #23 and it only applies
> > when we are on non-Windows OS, to give the window specific files enough time
> > to appear in case of a related bug. The code would get more complicated with
> > this sleep moved somewhere later and the result would be the same (same
> > sleep time) in the end.
> 
> No, it wont. We already wait for all the other files to appear. I don't want
> to loose that time by simply waiting that early. Also your if condition is
> wrong, as I have seen now. On Linux and Mac you will wait 6s but not on
> Windows.

That's the point.

The requirements were: on non-windows to wait at least 6 seconds and check that windows specific files are not present.

Comment 39

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

Oh, ok. So yes, do this sleep later. May be gcp can tell us how fast the files are usually written out once the first appears. Maybe we can reduce the sleep call a lot.

Comment 40

[Gian-Carlo Pascutto [:gcp]]

>May be gcp can tell us how fast the files are usually written out once the first appears.

See comment 25.

Comment 41

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

(In reply to Gian-Carlo Pascutto [:gcp] from comment #23)
> I wonder if it's worthwhile to add a check that the downloadwhite* files do
> not appear on non-Windows platforms? Though in that case, you do need to put
> a minimal delay before checking if they exist so it would break the above
> optimization again. 
> 
> So maybe wait 6s, check for files that should not be there (on not-Windows),
> and then check every second for at most 15s or so for the others to appear
> (all platforms). Sounds reasonable?

I don't think that this will work. The check for the files not being present on non-Windows systems should happen after all the checks for files being present. Otherwise it could still be that they are appearing a bit later on. 

Here my advise:

1. Wait for all the files which should be present via assert.waitFor()
2. On Linux and OS X do an extra minimal sleep maybe 2s should be enough
3. On Linux and OS X check that none of the files for Windows only are present

If the files already appear after 2s we could save about 4s in this test alone, given that mozmill itself already needs about 2s before the first test gets started.

Comment 42

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: test updated

Comment 43

[Andrei Eftimie]

Comment on attachment 8418683 [details] [diff] [review]
test updated

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

Looks good. I like how this looks with Henriks proposals implemented.

There are a few small things to fix:

Why have you moved this back in the functional testrun?
Has the decision been reversed to have this the first test of the structural testrun?

With all fixed please ask a review from Henrik.

::: firefox/tests/functional/restartTests/testSafeBrowsing_initialDownload/test2.js
@@ +21,5 @@
> +  { name: "goog-downloadwhite-digest256.pset", windowsOnly: true },
> +  { name: "goog-downloadwhite-digest256.sbstore", windowsOnly: true }
> +];
> +
> +// files which should appear an all OSes

I don't think these comments are needed, seems pretty clear from the variable names.

@@ +51,5 @@
> + */
> +function testSafeBrowsing() {
> +  allOSFiles.forEach(aFile => {
> +    assert.waitFor(() => aFile.file.exists(),
> +                     aFile.file.path + " exists", TIMEOUT);

nit: indentation

@@ +63,5 @@
> +  }
> +  else {
> +    controller.sleep(2000);
> +    winOnlyFiles.forEach(aFile => {
> +      assert.ok(!aFile.file.exists(), aFile.file.path + " was not downloaded");

"was downloaded"

Comment 44

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: test

Comment 45

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

(In reply to Andrei Eftimie from comment #43)
> Why have you moved this back in the functional testrun?
> Has the decision been reversed to have this the first test of the structural
> testrun?

No, we haven't made that decision. But it's a good question. Maybe we should consider it to be moved to the remote testrun. I planned to have structural testruns for tests which need some kind of setup done by Python scripts before. That is not the case here. We simply test remote data.

Do we know how much data we actually download for safe browsing in a functional testrun? I might want to go ahead and disable that feature by default in Mozmill, so our tests would have to enable it again.

Comment 46

[Gian-Carlo Pascutto [:gcp]]

>Do we know how much data we actually download for safe browsing in a functional testrun?

At most a few megabyte. I'd guess around ~1M for the initial update, a second update may come in if the test runs >45 minutes.

Comment 47

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

Oh, that's nothing. So we can leave it enabled I assume. Nothing to worry about.

Comment 48

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

Comment on attachment 8420072 [details] [diff] [review]
test

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

I was thinking more about the decision for a structural test, in terms if that is really necessary. Originally we wanted to have such a testrun for specific tests, which need special preparation done via a custom python script right before the test starts. Given that this is not the case here, and we only have to restart the browser to create a fresh profile, we should move it back to the remote testrun. It's not functional because we specifically test data coming from a remote website.

::: firefox/tests/structural/testSafeBrowsing_initialDownload/test1.js
@@ +6,5 @@
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  // close the browser to start with a new profile for the following test
> +  aModule.controller.stopApplication(true);

Please use a single test module. There is no need for multiple ones anymore in Mozmill 2.0. Also call restartApplication() here instead.

::: firefox/tests/structural/testSafeBrowsing_initialDownload/test2.js
@@ +31,5 @@
> +  var profile = Services.dirsvc.get("ProfD", Ci.nsIFile);
> +
> +  FILE_NAMES.forEach(aName => {
> +    let file = profile.clone();
> +    file.appendRelativePath("safebrowsing");

This line can be moved outside of the loop. It's constant.

@@ +39,5 @@
> +      winOnlyFiles.push({ file: file, name: aName });
> +    }
> +    else {
> +      allOSFiles.push({ file: file, name: aName });
> +    }

Why all that moving around, and not initially creating two different array constants?

Comment 49

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v8

Updated based on Henrik's review

Comment 50

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #48)
> ::: firefox/tests/structural/testSafeBrowsing_initialDownload/test1.js
> @@ +6,5 @@
> > +
> > +function setupModule(aModule) {
> > +  aModule.controller = mozmill.getBrowserController();
> > +  // close the browser to start with a new profile for the following test
> > +  aModule.controller.stopApplication(true);
> 
> Please use a single test module. There is no need for multiple ones anymore
> in Mozmill 2.0. Also call restartApplication() here instead.

`restartApplication()` doesn't clean the profile. We'll need to use `stopApplication(true)`

Comment 51

[Andrei Eftimie]

Comment on attachment 8422290 [details] [diff] [review]
v8

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

Just things. If we want a clean profile we'll need stopApplication(true).
Otherwise it doesn't make sense to have 2 different testfiles.

::: firefox/tests/remote/restartTests/testSafeBrowsing_initialDownload/test1.js
@@ +6,5 @@
> +
> +function setupModule() {
> +  let controller = mozmill.getBrowserController();
> +  // close the browser to start with a new profile for the following test
> +  controller.restartApplication();

This doesn't clean the profile.
Need to use stopApplication(true)

::: firefox/tests/remote/restartTests/testSafeBrowsing_initialDownload/test2.js
@@ +23,5 @@
> +  "goog-downloadwhite-digest256.pset",
> +  "goog-downloadwhite-digest256.sbstore"
> +];
> +
> +var filesLocation;

Remove this...

@@ +28,5 @@
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +
> +  filesLocation = Services.dirsvc.get("ProfD", Ci.nsIFile);

... and call this aModule.filesLocation

Comment 52

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

(In reply to Andrei Eftimie from comment #50)
> `restartApplication()` doesn't clean the profile. We'll need to use
> `stopApplication(true)`

Oh, that's true. I totally missed that point. Thanks for calling this out!

Comment 53

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v9

Updated based on previous review

Comment 54

[Andrei Eftimie]

Comment on attachment 8424773 [details] [diff] [review]
v9

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

Looks good to me. Just a small udpate.

With that please request a review from Henrik.

::: firefox/tests/remote/restartTests/testSafeBrowsing_initialDownload/test1.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +function setupModule() {

Please use aModule and use aModule.controller

Comment 55

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v9.1

Comment 56

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

Comment on attachment 8425408 [details] [diff] [review]
v9.1

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

There are some nits you should fix, but once done you have my r=me. Ask Andrei for the final review and landing.

::: firefox/tests/remote/restartTests/testSafeBrowsing_initialDownload/test1.js
@@ +7,5 @@
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  // close the browser to start with a new profile for the following test
> +  aModule.controller.stopApplication(true);
> +}

I'm ok with having two tests for now. But eventually we have to give testers the possibility to call stopApplication() with the next test function, while the profile gets reset. It involves a bit of logic, that's why it was disabled. Can you please check if a bug is filed on that? If not please get one created.

::: firefox/tests/remote/restartTests/testSafeBrowsing_initialDownload/test2.js
@@ +47,5 @@
> +    WIN_FILE_NAMES.forEach(aFileName => {
> +      let file = filesLocation.clone();
> +      file.appendRelativePath(aFileName);
> +
> +      assert.waitFor(() => file.exists(),

I would not use assert but expect here. So that we can test for all of the files, and don't fail right after the first failure.

@@ +49,5 @@
> +      file.appendRelativePath(aFileName);
> +
> +      assert.waitFor(() => file.exists(),
> +                     file.path + " exists", TIMEOUT);
> +    });

You have identical code here as above. Once that happens you should always create a helper method, which will simplify future updates of this test.

@@ +52,5 @@
> +                     file.path + " exists", TIMEOUT);
> +    });
> +  }
> +  else {
> +    controller.sleep(2000);

Please add a comment why we need this sleep.

@@ +57,5 @@
> +    WIN_FILE_NAMES.forEach(aFileName => {
> +      let file = filesLocation.clone();
> +      file.appendRelativePath(aFileName);
> +
> +      assert.ok(!file.exists(), file.path + " was not downloaded");

Same here regarding assert vs. expect.

Comment 57

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v10

Updated the test based on review and logged bug 1014544 for

(In reply to Henrik Skupin (:whimboo) from comment #56)
> Comment on attachment 8425408 [details] [diff] [review]
> v9.1
> 
> Review of attachment 8425408 [details] [diff] [review]:
> -----------------------------------------------------------------
> :::
> firefox/tests/remote/restartTests/testSafeBrowsing_initialDownload/test1.js
> @@ +7,5 @@
> > +function setupModule(aModule) {
> > +  aModule.controller = mozmill.getBrowserController();
> > +  // close the browser to start with a new profile for the following test
> > +  aModule.controller.stopApplication(true);
> > +}
> 
> I'm ok with having two tests for now. But eventually we have to give testers
> the possibility to call stopApplication() with the next test function, while
> the profile gets reset. It involves a bit of logic, that's why it was
> disabled. Can you please check if a bug is filed on that? If not please get
> one created.

Comment 58

[Andrei Eftimie]

Comment on attachment 8427002 [details] [diff] [review]
v10

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

Looks good.

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/e386857a0a2b (default)

Comment 59

[Andrei Eftimie]

Monica, would it help to backport this test on older branches?

Comment 60

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

Yes, we certainly want backports here.

Comment 61

[Gian-Carlo Pascutto [:gcp]]

Bug 997872 is in Firefox 30, not Firefox 31. I'm not too big a fan of aggressively uplifting SafeBrowsing changes (even if they're there to allow more testing).

Comment 62

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

(In reply to Gian-Carlo Pascutto [:gcp] from comment #61)
> Bug 997872 is in Firefox 30, not Firefox 31. I'm not too big a fan of

You might have mixed up numbers here. The patch on bug 997872 went into Firefox 31 but not 30. So I agree, we should not get this test backported to beta. Aurora is enough.

Comment 63

[Gian-Carlo Pascutto [:gcp]]

You're right, that should've read "31, not 30" of course.

Comment 64

[[:mmc] Monica Chew (no longer reading bugmail)]

Aurora sounds good to me.

Comment 65

[Mihaela Velimiroviciu (:mihaelav)]

I successfully applied the patch with the test on Aurora branch and got no failures when running it on my Ubuntu 13.04 machine with latest Aurora (31.0a2, 20140528004008)

Comment 66

[Andrei Eftimie]

Transplanted:
http://hg.mozilla.org/qa/mozmill-tests/rev/1f6746e50aeb (mozilla-aurora)

Comment 67

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

This should not have been backported to Aurora given the huge amount of failures we see on default. Please check that tests are passing before backporting them.