1548542 - Tests involving remote settings should not setup a Kinto server

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P3)

Description

[Mathieu Leplatre [:leplatrem]]

References:
https://searchfox.org/mozilla-central/rev/7944190ad1668a94223b950a19f1fffe8662d6b8/security/manager/ssl/tests/unit/test_intermediate_preloads.js#81

https://searchfox.org/mozilla-central/rev/7944190ad1668a94223b950a19f1fffe8662d6b8/security/manager/ssl/tests/unit/test_cert_storage.js#52-153

The above tests go too deep in terms of testing. If every use-case of Remote Settings replicates a whole synchronization process with fake server endpoints, it's going to be difficult for us to maintain.

Instead of running a fake Kinto server, testing the use-cases of Remote Settings should only involve what's exposed in the API, as for example storing local records in the local IDB or emitting fake "sync" events.

See https://firefox-source-docs.mozilla.org/services/common/services/RemoteSettings.html#unit-tests

Comment 1

[Mathieu Leplatre [:leplatrem]]

Attachment: Bug 1548542 - Remove notions of Kinto in blocklists unit tests

Comment 2

[Mathieu Leplatre [:leplatrem]]

So, basically I removed all the tests that were testing Remote Settings itself. We were in this situation for historical reasons: this blocklist implementation using Kinto precedes Remote Settings by years.

With the current patch, it's now a lot easier to read the tests and see what is actually tested.

For example, test_blocklist_pinning had no assertions about clearing the preloads on update (I added it). Likewise, test_cert_storage.js has no test about the different situations that a sync event can produce in the listener (create, update, delete, on empty db, etc.). test_blocklist_onecrl becomes useless and only asserts that a dump exists, since the rest is exactly the same as test_remote_settings with other identifiers.

It's also a lot easier to see the coupling between components. For example, we can see that cert_storage/lib.rs relies on a pref value stored by Remote Settings. This pref is not documented, hence it would be recommended to have an explicit operation to save the timestamp in the event listener (eg. certList.saveTimestamp()).

Bottom line, it gives it a new lease of life.

Note: I haven't touched test_intermediates_preloads.js because Bug 1488865 seems on the way. Please let me know if I can help there.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ee7e04b12dd3
Remove notions of Kinto in blocklists unit tests r=keeler,Gijs

Comment 4

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/ee7e04b12dd3