1304983 - Improve test_safe_browsing_initial_download.py to wait for all safebrowsing files being downloaded

Status: RESOLVED FIXED

(Testing :: Firefox UI Tests, defect)

Description

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

We have various test failures for safe browsing related tests at the moment with different files involved. To improve that me might not want to wait until a specific file has been downloaded but all of them. Reason is that there is a random order and with that a higher risk of failures. Once all files have been downloaded we can check for the presence for all of them.

(In reply to Dimi Lee[:dimi][:dlee] from bug 1304696 comment #5)
> Is it possible for ui-tests to check preference -
> "browser.safebrowsing.provider." + provider + ".lastupdatetime" is updated ?
> 
> For example , before update the value might be 1000, then a successful
> update will change that value.

Given that we are only interested in Google specific files being downloaded, I would say we stick with that for now. Francois, would you also be interested in other safebrowsing providers? What about our own files? Do we also need those?

In case we need multiple providers, is there a simple way to retrieve the list or would we have to parse the various preferences to get that?

Comment 1

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

Also I see that we have a preference which lists the appropriate files which is `browser.safebrowsing.provider.provider.lists`. Should we better use that value to check for downloaded files and not using a hardcoded list of files?

https://dxr.mozilla.org/mozilla-central/rev/f0e6cc6360213ba21fd98c887b55fce5c680df68/testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py

I think that would make the test easier. Also what about the different file extensions?

Comment 2

[François Marier [:francois]]

(In reply to Henrik Skupin (:whimboo) from comment #1)
> Also I see that we have a preference which lists the appropriate files which
> is `browser.safebrowsing.provider.provider.lists`. Should we better use that
> value to check for downloaded files and not using a hardcoded list of files?

I think that's a good approach, but that pref includes all of the possible lists for that provider, including lists that are disabled and not downloaded.

Instead, you should extract all of the list names from these prefs:

urlclassifier.blockedTable
urlclassifier.downloadAllowTable
urlclassifier.downloadBlockTable
urlclassifier.malwareTable
urlclassifier.phishTable
urlclassifier.trackingTable
urlclassifier.trackingWhitelistTable

and then you add these extensions:

- .sbstore
- .pset

Comment 3

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

From a discussion with Francois on IRC:

[10:37:24] whimboo francois so what about those entries... ^goog-badbinurl-shavar.cache$
[10:38:03] whimboo looks like another extension for the files beside sbstore and pset
[10:38:31] francois I think we can ignore the .cache files. As long as we have the other ones, we're doing well.
[10:38:46] whimboo are we creating those cache files on our own?
[10:41:33] francois I think they're not guaranteed to be there. Looking at my cache directory, I don't have them for the tracking protection list.
[10:41:49] whimboo maybe that is the reason for the failures then
[10:42:02] whimboo alsoin our current test we make a distinction for windows
[10:42:18] whimboo how do the urlcllassifier.* prefs cover that?
[10:42:19] francois That distinction is still valid. That's the urlclassifier.downloadAllowTable
[10:42:36] whimboo will that pref differ for windows?
[10:42:48] francois but if you just extract the list names from the prefs, you don't need to check the platform. That pref is empty on non-Windows platforms.
[10:42:58] whimboo great
[10:43:06] whimboo that's what I wanted to hear
[10:43:31] whimboo so lets go with this approach

Comment 4

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

Francois, I have working code locally except for the mozplugin-block-digest256.* files which are part of 'urlclassifier.blockedTable'. Do you know when those are getting downloaded? I have to say that even my profile which I use for daily work doesn't contain those files.

Comment 5

[François Marier [:francois]]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> Francois, I have working code locally except for the
> mozplugin-block-digest256.* files which are part of
> 'urlclassifier.blockedTable'. Do you know when those are getting downloaded?
> I have to say that even my profile which I use for daily work doesn't
> contain those files.

You'll need to enable this pref if it's not already set to "true":

    browser.safebrowsing.blockedURIs.enabled

Comment 6

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

Attachment: Bug 1304983 - Improve test_safe_browsing_initial_download.py to wait for all safebrowsing files being downloaded.

Review commit: https://reviewboard.mozilla.org/r/80752/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80752/

Comment 7

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

Comment on attachment 8794241 [details]
Bug 1304983 - Improve test_safe_browsing_initial_download.py to wait for all safebrowsing files being downloaded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80752/diff/1-2/

Comment 8

[François Marier [:francois]]

Comment on attachment 8794241 [details]
Bug 1304983 - Improve test_safe_browsing_initial_download.py to wait for all safebrowsing files being downloaded.

https://reviewboard.mozilla.org/r/80752/#review80392

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15c8100e56db
Improve test_safe_browsing_initial_download.py to wait for all safebrowsing files being downloaded. r=francois

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/15c8100e56db

Comment 11

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

According to the discussion on bug 1304781 we want to have those test changes uplifted to aurora for better failure details. Thanks.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/88e2a41ad412