Closed Bug 664932 Opened 9 years ago Closed 8 years ago

Update unit tests must stop implementing nsIXMLHttpRequestEventTarget in script


(Toolkit :: Application Update, defect)

Not set





(Reporter: sicking, Assigned: sicking)




(1 file)

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?
Attached patch Patch to fixSplinter Review
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.
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.