1347422 - RemoveDataFromDomain tests do not yield on it

Status: RESOLVED INVALID

(Firefox :: General, defect)

Description

[Milind L (:milindl)]

Unit tests for ForgetAboutSite.removeDataFromDomain should yield on it.

Comment 1

[Milind L (:milindl)]

Attachment: 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/

Comment 2

[Milind L (:milindl)]

(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 :)

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

IS this ready for review?

Comment 4

[Milind L (:milindl)]

(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 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 6

[Milind L (:milindl)]

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()

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(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.

Comment 8

[Milind L (:milindl)]

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.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 10

[Milind L (:milindl)]

Comment on attachment 8847423 [details]
Bug 1347422 - yield on removeDataFromDomain in tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120394/diff/1-2/

Comment 11

[Milind L (:milindl)]

(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

Comment 12

[Milind L (:milindl)]

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

Comment 13

[Milind L (:milindl)]

Closing this bug and shifting patch to the 'parent' bug as requested by Marco.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8847423 [details]
Bug 1347422 - yield on removeDataFromDomain in tests

https://reviewboard.mozilla.org/r/120394/#review123360