RemoveDataFromDomain tests do not yield on it

RESOLVED INVALID

Status

()

defect
RESOLVED INVALID
2 years ago
2 years ago

People

(Reporter: milindl, Assigned: milindl, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Unit tests for ForgetAboutSite.removeDataFromDomain should yield on it.
(Assignee)

Updated

2 years ago
Blocks: 1247201
Comment hidden (mozreview-request)
(Assignee)

Comment 2

2 years ago
(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
(Assignee)

Comment 4

2 years ago
(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

2 years ago
mozreview-review
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
(Assignee)

Comment 6

2 years ago
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.
(Assignee)

Comment 8

2 years ago
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
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 years ago
(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
(Assignee)

Comment 12

2 years ago
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
(Assignee)

Comment 13

2 years ago
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 14

2 years ago
mozreview-review
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.