Opened 6 months ago

Closed 6 months ago

Last modified 13 days ago

#7457 closed defect (fixed)

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

Reported by: kzar Assignee: kzar
Priority: P2 Milestone: Adblock-Plus-for-Chrome-Opera-Firefox-next
Module: Platform Keywords:
Cc: sebastian, tlucas Blocked By:
Blocking: Platform: Firefox
Ready: yes Confidential: no
Tester: Unknown Verified working: yes
Review URL(s):

https://gitlab.com/eyeo/adblockplus/adblockpluschrome/merge_requests/66

Description (last modified by 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.

Change History (8)

comment:1 follow-up: Changed 6 months ago by 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!

comment:2 in reply to: ↑ 1 Changed 6 months ago by 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.

Last edited 6 months ago by sebastian (previous) (diff)

comment:3 Changed 6 months ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:4 Changed 6 months ago by abpbot

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

comment:5 Changed 6 months ago by kzar

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:6 Changed 6 months ago by kzar

  • Description modified (diff)

comment:7 Changed 6 months ago by cee1994

spam

Last edited 13 days ago by kzar (previous) (diff)

comment:8 Changed 3 months ago by ukacar

  • Verified working set
Note: See TracTickets for help on using tickets.