59 bytes, text/x-review-board-request
Unit tests for ForgetAboutSite.removeDataFromDomain should yield on it.
(In reply to milindl from comment #1) > Created attachment 8847423 [details] > Bug 1347422 - yield on removeDataFromDomain in tests > > Review commit: https://reviewboard.mozilla.org/r/120394/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/120394/ Marco, The tests were already using add_task, since a few of them were generators, so converting the rest and yielding was not very hard. Thanks :)
IS this ready for review?
Assignee: nobody → i.milind.luthra
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(In reply to Marco Bonardo [::mak] from comment #3) > IS this ready for review? Yes, it is. As I said, the changes are minimal since we are using add_task already.
Comment on attachment 8847423 [details] Bug 1347422 - yield on removeDataFromDomain in tests https://reviewboard.mozilla.org/r/120394/#review122448 This is not the only test using removeDataFromDomain, see http://searchfox.org/mozilla-central/search?q=removeDataFromDomain&path=test
Marco, I need a clarification on some of these tests (which use test()) * Is it OK to promisify some other functions local to the file? For example, in browser/components/sessionstore/test/browser_394759_purge.js, waitForClearHistory() takes a callback. It would be much nicer if I could return a promise and yield on it inside my add_task()
(In reply to milindl from comment #6) > Marco, I need a clarification on some of these tests (which use test()) > > * Is it OK to promisify some other functions local to the file? Absolutely yes, as far as the test keeps its scope, it's fine to modify it.
Marco, Thanks for the clarification; I'm mostly done with it, I'll put a WIP patch up as soon as I run all the affected tests (the build is taking some time since I needed to clobber again). Meanwhile, please take a look at https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/test/browser_forgetThisSite.js It follows a different pattern of testing (each subtest calls the other one thru a call to executeSoon). Some other browser_ tests in that directory also follow a similar format. Also, it uses a function from its head.js called setFinishedCallback, which I'm not sure I understand a 100% ( I'm trying to fix that though :) ) Basically, I'm not sure how I need to restructure that particular file.
In those cases it's better to limit to the minimum changes needed, or you may end up rewriting the whole test, that is probably not what you want. if it's using callbacks, it may be needed to use the promise .then() method to invoke callbacks in the correct order. Basically for that case you can make test3 using .then() from removeDataFromDomain and invoke the executesoon inside its callback to move to test4
(In reply to Marco Bonardo [::mak] from comment #9) > In those cases it's better to limit to the minimum changes needed, or you > may end up rewriting the whole test, that is probably not what you want. Addresses my concern, that's what I was worried about. Please see the review: it's still work in progress. In particular, I have a few problems: * https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/browser_464199.js -- fails with the message : Found an unexpected browser window at the end of test run * I am unsure why this is happenening, since I am yielding on the closeWindow function * Do I still need to finish(); if I am using add_task? Thank you
Also, in https://dxr.mozilla.org/mozilla-central/source/toolkit/forgetaboutsite/test/browser/browser_clearplugindata.js#84 The test fails. Surprisingly, I have not touched this line. Also, if you change |null| to ["localhost"], the test passes. I'm 100% in the dark to what's going on. Thanks
Closing this bug and shifting patch to the 'parent' bug as requested by Marco.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INVALID
Comment on attachment 8847423 [details] Bug 1347422 - yield on removeDataFromDomain in tests https://reviewboard.mozilla.org/r/120394/#review123360
Attachment #8847423 - Flags: review?(mak77)
You need to log in before you can comment on or make changes to this bug.