One of three things keeping aurora closed after the merge. Every flavor of Linux, local and remote tests, e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=71230736&repo=mozilla-aurora 17:37:53 INFO - TEST-UNEXPECTED-FAIL | test_safe_browsing_initial_download.py TestSafeBrowsingInitialDownload.test_safe_browsing_initial_download | AssertionError: Items in the first set but not the second: 17:37:53 INFO - 'google4'

Henrik, could you take a look at this, its one of the tree closing reasons for aurora

Francois and Henry were working on changes lately. They might better know what has been changed.

Maybe this is somewhat related to bug 1330253.

Does it only fail on aurora? The test case is written under the consumption that google api key is not properly set on the machines. Henrik, do you know if aurora is running with google api key or not?

yeah its so far only perma failing on aurora after yesterdays merge day

Same seems to happen for all the nightly build tests as run via mozmill-ci on mozilla-aurora: https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&filter-searchStr=Firefox%20UI&filter-tier=3 I would suggest that we disable this test completely for now until a solution is ready in how to handle the Google v4 lists. I can create the skip patch.

As the log says we downloaded: "goog-phish-shavar,goog-malware-shavar,goog-unwanted-shavar,goog-badbinurl-shavar". When I have a look at the table preferences in about:config I can see way more files listed there. For example I do not see any proto file downloaded. I assume that this might be the problem here, which is not related to a missing API key.

(In reply to Henrik Skupin (:whimboo) from comment #8) > As the log says we downloaded: > "goog-phish-shavar,goog-malware-shavar,goog-unwanted-shavar,goog-badbinurl- > shavar". When I have a look at the table preferences in about:config I can > see way more files listed there. For example I do not see any proto file > downloaded. I assume that this might be the problem here, which is not > related to a missing API key. Those downloaded tables are all NOT v4. V4 tables will be suffixed by "-proto".

Skip patch for aurora. It might be a problem in Firefox through, but that is something I cannot answer.

(In reply to Henry Chang [:henry][:hchang] from comment #9) > Those downloaded tables are all NOT v4. V4 tables will be suffixed by > "-proto". Then none of them are actually getting downloaded.

landed in https://hg.mozilla.org/releases/mozilla-aurora/rev/41a10c805351

(In reply to Henry Chang [:henry][:hchang] from comment #4) > Henrik, do you know if aurora is running with google api key or not? I don't know. This is more a question for Dustin.

It seems like there are two problems with the test: 1. This line is in the wrong part of the `if` statement (it should be with the V4 files): http://searchfox.org/mozilla-central/rev/02a56df6474a97cf84d94bbcfaa126979970905d/testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py#61 2. This line should be commented out until bug 1330253 is fixed: http://searchfox.org/mozilla-central/source/testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py#83 That should ensure that we don't look for the non-existent V4 files in aurora.

(In reply to François Marier [:francois] from comment #14) > It seems like there are two problems with the test: > > 1. This line is in the wrong part of the `if` statement (it should be with > the V4 files): > > http://searchfox.org/mozilla-central/rev/ > 02a56df6474a97cf84d94bbcfaa126979970905d/testing/firefox-ui/tests/functional/ > security/test_safe_browsing_initial_download.py#61 > > 2. This line should be commented out until bug 1330253 is fixed: > http://searchfox.org/mozilla-central/source/testing/firefox-ui/tests/ > functional/security/test_safe_browsing_initial_download.py#83 > I don't think it matters because |self.safebrowsing_v4_files| is never used after initialization. But it'd be better to be commented out until Bug 1330253 is fixed. > That should ensure that we don't look for the non-existent V4 files in > aurora. Per offline discussion with Francois, there are in total 4 combinations: 1) Whether v4 tables (*-proto) are in 'urlclassifier.malwareTable' and 'urlclassifier.phishTable' The current configuration is: only on Nightly build will there be v4 tables. The presence of v4 tables will lead to creating 'google4' directory no matter the tables can be actually downloaded. 2) Whether the build is built on the machine which has google api key or not. So, a) On try builds, we have the v4 tables and NO google api key. The current test case is written to adapt this case. b) On aurora builds, I can only assured that v4 tables are not in the prefs. so that [1] should be removed as what Francois metioned in comment 14. What I am not sure is whether the aurora testing builds are built on machines which have a google api key. I think I can figure out this by checking the artifacts. Chances are we build aurora for testing on some machines and build aurora for release on others. (The aurora for release will definitely have a google api key.) [1] http://searchfox.org/mozilla-central/rev/02a56df6474a97cf84d94bbcfaa126979970905d/testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py#61

By checking [1] an arbitrary aurora build [2], which was actually used to run test cases, it's confirmed that the google api key is NOT built into. So I can conclude the testing failures is solely because of the absence of v4 tables on aurora builds. [1] Download the zip file, decompress omni.ja and check nsURLFormatter.js. [2] https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-aurora-linux-debug/1484751206/firefox-52.0a2.en-US.linux-i686.tar.bz2

Should this block the 53 release? I would assume so, or? We may need a product related bug then.

(In reply to Henrik Skupin (:whimboo) from comment #13) > (In reply to Henry Chang [:henry][:hchang] from comment #4) > > Henrik, do you know if aurora is running with google api key or not? > > I don't know. This is more a question for Dustin. Aurora is a level-3 repo, so it has access to the secret. Whether it actually *uses* that secret should be discernible from the mozharness logs (in the get-secrets step). I don't know if the recent change to that mozharness configuration is merged to aurora. It looks like the conversation has moved on from this question, so I didn't dig, but I can help out if necessary.

(In reply to Dustin J. Mitchell [:dustin] from comment #18) > (In reply to Henrik Skupin (:whimboo) from comment #13) > > (In reply to Henry Chang [:henry][:hchang] from comment #4) > > > Henrik, do you know if aurora is running with google api key or not? > > > > I don't know. This is more a question for Dustin. > > Aurora is a level-3 repo, so it has access to the secret. Whether it > actually *uses* that secret should be discernible from the mozharness logs > (in the get-secrets step). I don't know if the recent change to that > mozharness configuration is merged to aurora. It looks like the > conversation has moved on from this question, so I didn't dig, but I can > help out if necessary. Per my investigation in comment 16, the aurora build 'for testing' has no google api key in the current configuration.

What's the best way to test this patch on the aurora test machines?

(In reply to François Marier [:francois] from comment #22) > Comment on attachment 8830569 [details] > Bug 1333303 - Remove the google4 dir from the test unless there are V4 lists. > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/107316/diff/2-3/ I guess this change will make the test case fail on try cuz v4 tables will be present on try build.

(In reply to François Marier [:francois] from comment #23) > What's the best way to test this patch on the aurora test machines? I don't know if this is the equivalence: Check out the mozilla-aurora repo and just push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=538de08c7f4d9474691cfbef7f0d6138895a0b21

Comment on attachment 8830569 [details] Bug 1333303 - Remove the google4 dir from the test and relax the list comparison. https://reviewboard.mozilla.org/r/107316/#review108548 I don't think that I'm a good reviewer for this given that I do not know the logic in how it should all work. I think it's better to let Henry review those changes.

FYI the submitted try build shows a linting failure for the test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=538de08c7f4d9474691cfbef7f0d6138895a0b21&selectedJob=72124889

(In reply to Henrik Skupin (:whimboo) from comment #26) > I don't think that I'm a good reviewer for this given that I do not know the > logic in how it should all work. I think it's better to let Henry review > those changes. Unfortunately, Henry will be away until 6 Feb because of Chinese New Year. I'd like to get this fixed sooner so that we can have Safe Browsing tests running on aurora again. I re-did the logic and tested it on both central and aurora. It should all work now and also be more tolerant of extra files being around. Essentially, I'm listing the files in the directory on disk and I'm making sure that all of the expected V2 files are present. Hopefully that will be enough for you to review the Python stuff?

Try run on mozilla-central: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd546a080e4c97d6804e0a9da82d6c4488d63473 Try run on aurora: https://treeherder.mozilla.org/#/jobs?repo=try&revision=67f52da53e1e5df656814853bebe76ea14999f30

Comment on attachment 8830569 [details] Bug 1333303 - Remove the google4 dir from the test and relax the list comparison. https://reviewboard.mozilla.org/r/107316/#review108878 ::: testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py:59 (Diff revision 4) > my_file_extensions = self.v4_file_extensions > else: # v2 > - # safebrowsing dir should have a 'google4' directory where > - # v4 databases exist. > - files.append('google4') > my_file_extensions = self.v2_file_extensions So it means when v4 is active none of the v2 files are getting served, correct? ::: testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py:64 (Diff revision 4) > my_file_extensions = self.v2_file_extensions > > for pref_name in self.prefs_download_lists: > base_names = self.marionette.get_pref(pref_name).split(',') > for ext in my_file_extensions: > files.extend(['{file}.{ext}'.format(file=f, ext=ext) Please try to avoid `file`. For the formatting here you can also use `{0}.{1}` given that it is not that complex. Otherwise use `name`. ::: testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py:83 (Diff revision 4) > 'safebrowsing') > self.safebrowsing_v2_files = self.get_safebrowsing_files(False) > - self.safebrowsing_v4_files = self.get_safebrowsing_files(True) > + # Bug 1330253 - Leave the next line disabled until we have google API key > + # on the CI machines. > + # self.safebrowsing_v4_files = self.get_safebrowsing_files(True) > + self.safebrowsing_v4_files = [] Why do we have to keep the v2 logic at all? Can it be that we are switching forth and back between those two versions? ::: testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py:103 (Diff revision 4) > Wait(self.marionette, timeout=60).until( > check_downloaded, message='Not all safebrowsing files have been downloaded') > finally: > - self.assertSetEqual(self.safebrowsing_v2_files, > - set(os.listdir(self.safebrowsing_path))) > - # Bug 1330253 - Leave the next test disabled until we have google api key > + files_on_disk_toplevel = os.listdir(self.safebrowsing_path) > + for f in self.safebrowsing_v2_files: > + self.assertIn(f, files_on_disk_toplevel) Why not simply `self.assertIn(self.safebrowsing_v2_files, files_on_disk_toplevel)`? Or do you want to make the failure message better readable? ::: testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py:108 (Diff revision 4) > - # Bug 1330253 - Leave the next test disabled until we have google api key > - # on the CI machines. > - # self.assertSetEqual(self.safebrowsing_v4_files, > - # set(os.listdir(os.path.join(self.safebrowsing_path, 'google4')))) > + self.assertIn(f, files_on_disk_toplevel) > + > + if len(self.safebrowsing_v4_files) > 0: > + files_on_disk_google4 = os.listdir(os.path.join(self.safebrowsing_path, 'google4')) > + for f in self.safebrowsing_v4_files: > + self.assertIn(f, files_on_disk_google4) Same as above. Also it would be nice to have a more data-driven test so that we won't need all this logic twice. But this could be done in a follow-up bug.

Comment on attachment 8830569 [details] Bug 1333303 - Remove the google4 dir from the test and relax the list comparison. https://reviewboard.mozilla.org/r/107316/#review108878 > So it means when v4 is active none of the v2 files are getting served, correct? Not quite. This function is called once with `is_v4 == false`, and then once more with `is_v4 == true` (if V4 is enabled). So we always look for the V2 lists. > Why do we have to keep the v2 logic at all? Can it be that we are switching forth and back between those two versions? For Google lists, both V2 and V4 are enabled in Nightly. Our plan is to migrate to V4 eventually, but we'll be running them in parallel for a little while until we are satisfied with the new code. Longer term, we'll be using V4 for the Google lists and V2 for the tracking protection lists. The server we use to serve the tracking protection lists is V2 only and I'm not aware of any plans to upgrade it.

Comment on attachment 8830569 [details] Bug 1333303 - Remove the google4 dir from the test and relax the list comparison. https://reviewboard.mozilla.org/r/107316/#review108878 > Why not simply `self.assertIn(self.safebrowsing_v2_files, files_on_disk_toplevel)`? > > Or do you want to make the failure message better readable? Yes, I wanted to make it easier to spot the missing file. Comparing entire sets gets us very lengthy error messages. > Same as above. Also it would be nice to have a more data-driven test so that we won't need all this logic twice. But this could be done in a follow-up bug. Yeah it might be better not to do to much refactoring in this bug since I would like to uplift to aurora too.

Comment on attachment 8830569 [details] Bug 1333303 - Remove the google4 dir from the test and relax the list comparison. https://reviewboard.mozilla.org/r/107316/#review110984

Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c5621cb6f907 Remove the google4 dir from the test and relax the list comparison. r=whimboo

Francois, I think for Aurora we need a different patch which also enables the test again? Otherwise we could land by backing out the skip patch.

Approval Request Comment [Feature/Bug causing the regression]: This test was disabled because it was permaorange [User impact if declined]: none [Is this code covered by automated tests?]: n/a [Has the fix been verified in Nightly?]: the test passes in Nightly [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none, this includes the backout of the skip patch [Is the change risky?]: no [Why is the change risky/not risky?]: test only [String changes made/needed]: none

Try run for the aurora patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=62190593dbbbbc6143ee18787b91b5a30f65a27d

Comment on attachment 8835335 [details] [diff] [review] bug1333303-aurora.patch Fix a test failure. Aurora53+.

Modified from Uplift Dashboard.

François, can you please rebase this patch for mozilla-beta when you get a chance? We hit this permafail on the uplift of bug 1333483 there as well. Thanks!

It appears to me that the v4 bits of this patch are dependent on Bug 1305486, which added support for v4. Since Bug 1305486 was not merged into Firefox 52 (and I would prefer not to uplift it if possible), I wrote a version of the patch that relaxes the test comparison, but doesn't deal with v4. @francois Does this look ok to you? Could/should we uplift this to beta?

Comment on attachment 8837735 [details] [diff] [review] Relax test comparison (beta uplift) test-only changes don't need approval. Thanks for doing the rebase!

Comment on attachment 8837735 [details] [diff] [review] Relax test comparison (beta uplift) Could a sheriff check this into beta for me?