Permafailing test_safe_browsing_initial_download.py TestSafeBrowsingInitialDownload.test_safe_browsing_initial_download | AssertionError: Items in the first set but not the second: on aurora 53

RESOLVED FIXED in Firefox 52

Status

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: philor, Assigned: francois)

Tracking

53 Branch
mozilla54
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

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

Details

(Whiteboard: [stockwell fixed])

Attachments

(5 attachments)

Reporter

Description

2 years ago
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
Flags: needinfo?(hskupin)
Francois and Henry were working on changes lately. They might better know what has been changed.
Flags: needinfo?(hskupin)
Flags: needinfo?(hchang)
Flags: needinfo?(francois)
Maybe this is somewhat related to bug 1330253.
Flags: needinfo?(hchang)
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?
Flags: needinfo?(hskupin)
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.
Flags: needinfo?(hskupin)
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.
Whiteboard: [test disabled on aurora]
(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.
Flags: needinfo?(dustin)
Assignee

Comment 14

2 years ago
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.
Flags: needinfo?(francois)
(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.
Flags: needinfo?(dustin)
(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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 23

2 years ago
What's the best way to test this patch on the aurora test machines?
Assignee: nobody → francois
Status: NEW → ASSIGNED
(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 26

2 years ago
mozreview-review
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.
Attachment #8830569 - Flags: review?(hskupin)
Comment hidden (mozreview-request)
Assignee

Comment 29

2 years ago
(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?

Comment 31

2 years ago
mozreview-review
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.
Attachment #8830569 - Flags: review?(hskupin)
Assignee

Comment 32

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Assignee

Comment 34

2 years ago
mozreview-review-reply
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.
Reporter

Updated

2 years ago
Duplicate of this bug: 1336880
Reporter

Updated

2 years ago
Severity: blocker → normal

Comment 36

2 years ago
mozreview-review
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
Attachment #8830569 - Flags: review?(hskupin) → review+

Comment 37

2 years ago
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

Comment 38

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c5621cb6f907
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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.
Flags: needinfo?(francois)
Assignee

Updated

2 years ago
Duplicate of this bug: 1336895
Assignee

Comment 41

2 years ago
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
Flags: needinfo?(francois)
Attachment #8835335 - Flags: approval-mozilla-aurora?
Comment on attachment 8835335 [details] [diff] [review]
bug1333303-aurora.patch

Fix a test failure. Aurora53+.
Attachment #8835335 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Modified from Uplift Dashboard.
Whiteboard: [test disabled on aurora]
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!
Flags: needinfo?(francois)
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?
Attachment #8837735 - Flags: review?(francois)
Assignee

Updated

2 years ago
Attachment #8837735 - Flags: review?(francois) → review+
Flags: needinfo?(francois)
Attachment #8837735 - Flags: approval-mozilla-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!
Attachment #8837735 - Flags: approval-mozilla-beta?
Comment on attachment 8837735 [details] [diff] [review]
Relax test comparison (beta uplift)

Could a sheriff check this into beta for me?
Attachment #8837735 - Flags: checked-in?
Whiteboard: [checkin-needed-beta]
Attachment #8837735 - Flags: checked-in?
Whiteboard: [stockwell fixed]
You need to log in before you can comment on or make changes to this bug.