Closed Bug 664932 Opened 9 years ago Closed 8 years ago
Update unit tests must stop implementing ns
IXMLHttp Request Event Target in script
Currently the updater unit tests implement a custom nsIXMLHttpRequest while running the tests. What's worse is that it also implements nsIXMLHttpRequestEventTarget which is getting marked as a 'builtinclass' in bug 661980 which means that it can't be implemented in script. We should soon be doing the same thing with nsIXMLHttpRequest since it should be extending nsIXMLHttpRequestEventTarget according to DOM specs. So we need to find another way of implementing these tests. A few options are: * Use the normal XHR implementation instead? * Add a new interface with the same properties as nsIXMLHttpRequest/nsIXMLHttpRequestEventTarget. (We'd also have to change the actual upload code to use nsISupports as interface argument to CreateInstance * Skip going through XPConnect somehow, which would remove the need for implementing any XPCOM interfaces at all.
I went with a custom nsIXMLHttpRequest implementation since I wasn't able to test all of the error conditions and it had the added benefit of it it being much faster. I can use the normal implementation but I don't know of a way to coerce nsIXMLHttpRequest to return these errors. I would feel better if the error conditions were tested by the nsIXMLHttpRequest test... I added a couple in bug 487571 but there are many more that I don't know of a way to add. Is that possible?
I went for the third bullet point from comment 0. The downside is that this does require a small modification to the shipped code, but it should hopefully be safe.
Assignee: nobody → jonas
Attachment #540192 - Flags: review?(robert.bugzilla)
To answer your questions about more XHR tests. I honestly don't know what type of error situations we can simulate in for example mochitest. We do actually have a real networking team now though, so I bet they can help with figuring it out. Would be great if you could file a new bug (cc me and :smaug) and mention any specific things that you know of that needs testing.
Attachment #540192 - Flags: review?(robert.bugzilla) → review+
Checked in! http://hg.mozilla.org/mozilla-central/rev/56854b8cf1a6 Thanks for the quick review!
8 years ago
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.