"Property-wise modification" prefs qunit test failing on Firefox 66

[kzar]

Environment

Firefox 66.0.2 Adblock Plus built from ​current next.

How to reproduce

1. Open the Adblock Plus options page
2. navigate to qunit/index.html

Observed behaviour

The "Property-wise modification" prefs.js tests fail.

Expected behaviour

All the tests pass.

Hints for testers

Nothing to test.

[kzar]

To me it seems like almost all of these tests are broken.

When assigning a preference value (e.g. Prefs.notification = "foo"), a setter is called which in turns calls Prefs.set. Prefs.set puts the value in our overrides Object synchronously, then returns a Promise which resolves when the value is written to browser.local.storage asynchronously.

The preference getters simply read the value back from our overrides Object synchronously. That way, we don't have to worry if we've finished writing a preference's value to browser.local.storage before checking its value.

Anyway, these tests seem to get all this confused. They assign preferences using the setters, then without having any idea if writing to browser.local.storage is finished, they attempt to read the value back from browser.local.storage manually.

We should be probably either calling Prefs.set explicitly and waiting for the returned Promise to resolve before reading the value back from browser.local.storage, or read the value back using the preference getters.

Furthermore, the test in question seems misleading and pointless. It's described as "Property-wise modification", as opposed to the previous test "Object pref (complete replacement)". While it does assign properties on the preference's value, it actually only then stores the value by turning the Object to a JSON string and back, before reassigning the whole thing!

[sebastian]

Replying to kzar:

We should be probably either calling Prefs.set explicitly and waiting for the returned Promise to resolve before reading the value back from browser.local.storage, or read the value back using the preference getters.

If we call Prefs.set() directly, the code path of the setter wouldn't be tested. If we only test the getter, we don't test whether the data are actually persisted. Maybe the best approach would be to retry browser.storage.get() for up to 1s (or something like that).

Furthermore, the test in question seems misleading and pointless. It's described as "Property-wise modification", as opposed to the previous test "Object pref (complete replacement)". While it does assign properties on the preference's value, it actually only then stores the value by turning the Object to a JSON string and back, before reassigning the whole thing!

That is a pattern we use in our actual source code (when saving the notificationdata preference). It is a hack, but unless we assign a new object, the preference isn't written to storage.

[abpbot]

A commit referencing this issue has landed:
​Issue 7457 - Rewrite the preference storage tests

[cee1994]

spam