Closed Bug 1011813 Opened 11 years ago Closed 11 years ago

make test addon for native options unload

Categories

(Add-on SDK Graveyard :: General, defect, P1)

Product:
Add-on SDK Graveyard
Component:
General
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: zombie, Assigned: evold)

References

Details

Attachments

(1 file, 1 obsolete file)

Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1504
46 bytes, text/x-github-pull-request
jsantell
: review+
Details | Review
my understanding is this test addon must land separately after native options are uplifted, see bug 903039 comment 24 for more details.
Comment on attachment 8424471 [details] [review] Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1493 simply extracted from my previous PR. Erik, is this ok, or did you have something else/different in mind?
Attachment #8424471 - Flags: review?(evold)
(In reply to Tomislav Jovanovic [:zombie] from comment #2) > Comment on attachment 8424471 [details] [review] > Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1493 > > simply extracted from my previous PR. > > Erik, is this ok, or did you have something else/different in mind? Ya I did, do you mind if I take this one?
Flags: needinfo?(tomica+amo)
Comment on attachment 8424471 [details] [review] Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1493 no, be my guest..
Attachment #8424471 - Attachment is obsolete: true
Attachment #8424471 - Flags: review?(evold)
Flags: needinfo?(tomica+amo)
Assignee: nobody → evold
Blocks: sdk/simple-prefs
From bug 903039 comment 15: I'm also concerned that the AOM could change one day by caching these inline option documents and still fire the event which we use to update them, in which case we wouldn't have any tests fail but there would be duplicate <setting> nodes, so we need some tests to protect ourselves against this. Either we can handle that possible case, or we can add a test that this is not the case, which would fail if this assumption changes.
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #5) > From bug 903039 comment 15: > > I'm also concerned that the AOM could change one day by caching these inline > option documents and still fire the event which we use to update them, in > which case we wouldn't have any tests fail but there would be duplicate > <setting> nodes, so we need some tests to protect ourselves against this. > Either we can handle that possible case, or we can add a test that this is > not the case, which would fail if this assumption changes. Oh I think we have this test, bug 903039 comment 20 is the description of the test needed here.
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #6) > Oh I think we have this test, bug 903039 comment 20 is the description of > the test needed here. yes, we do. https://github.com/mozilla/addon-sdk/pull/1368/files#diff-16
Summary: add follow-up test addon for native options → make test addon for native options unload
Haven't looked through all of the PR yet, but is there a reason we're landing a zipwriter in the SDK outside of tests? Couldn't we have this as `./test/utils/zip`, rather than exposing it as a module?
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #9) > Haven't looked through all of the PR yet, but is there a reason we're > landing a zipwriter in the SDK outside of tests? Couldn't we have this as > `./test/utils/zip`, rather than exposing it as a module? No reason, I took it from the cfx-js branch which had it in `sdk/zip`. I suppose it's better to move it to the test directory for now, and JEP a zip api.
Some nits -- I'd say the zip and preferences/utils modules should be moved into a test utils directory or something so it's not something we have to support/discuss, and can promote it as a "public" module if we need to. I don't understand why `promises` were removed from the promise test suite as well. Other than that, r+
Attachment #8432326 - Flags: review?(jsantell) → review+
I addressed the review comments, I couldn't move the preferences/utils file without duplicating it, so I'd rather not do that. Is this alright with you? I'm pretty sure Irakli is alright stuff like this in low level apis.
Flags: needinfo?(jsantell)
Yeah looks good
Flags: needinfo?(jsantell)
Depends on: 1024837
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: