784870 - Need stub for nsIFile to make test efficient

Status: NEW

(MailNews Core :: Testing Infrastructure, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Bug 674742 is caused by the failure of nsIFile.moveToNative in nsFolderCompactState::FinishCompact(). To write a test for the bug, we need to raise the failure in the test.

Fortunately nsIFile is an interface, so we can implement a test specific nsIFile which causes the failure of nsIFile.moveToNative.

Unfortunately the test specific nsIFile can not be implemented now because nsIFile is 'builtinclass'. I've already opened a bug for it (bug 783414).

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Proposed a stub for nsIFile

Comment 2

[Kent James (:rkent)]

Hiro, you are a braver man than I am to try to push through bug 783414. I doubt very seriously whether you will be able to get the core folks to accept a change to such an essential interface as nsIFile.

Still, I think it is a worthwhile effort to consider how to simulate file operation failures in unit testing. Perhaps this bug could morph into a consideration of options?

Have you tried Benjamin's suggestion in https://bugzilla.mozilla.org/show_bug.cgi?id=783414#c4 to use read-only properties of the directory to simulate that?

The other approach that I was considering was to add a testing method to the compact object itself. You could add a flag that would be set by a unit test that would (falsely) return an error when in fact the file operation succeeded. That is somewhat artificial, but so is your approach of a stub function.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Kent James (:rkent) from comment #2)
> Have you tried Benjamin's suggestion in
> https://bugzilla.mozilla.org/show_bug.cgi?id=783414#c4 to use read-only
> properties of the directory to simulate that?

Yes, I tried. But it can not be used as I commented in bug 783414 #c5.

> The other approach that I was considering was to add a testing method to the
> compact object itself. You could add a flag that would be set by a unit test
> that would (falsely) return an error when in fact the file operation
> succeeded. That is somewhat artificial, but so is your approach of a stub
> function.

I think adding the method only for test is a last resort. 

If nsIFile (and nsIXXStreams) interfaces can be implemented in xpcshell test, we can write lots of test cases for I/O errors. I am convinced that those test cases make Thunderbird more stable especially in dataloss cases.

Comment 4

[Mark Banner (:standard8)]

I'd really like to try and avoid a stub. The problem generally is, at some stage nsIFile will change and the result will break us, this means we then have to update our code to fix the stub, whereas otherwise we might have got away with it if we didn't use that function. I really want to avoid introducing dependencies such as this, and we've been working away from them.

Maybe you could post your approach of trying to set the file/directory as read-only, and point out where it failed, so we can take a look and see the issue?

Comment 5

[Hiroyuki Ikezoe (:hiro)]

(In reply to Mark Banner (:standard8) from comment #4)
> I'd really like to try and avoid a stub. The problem generally is, at some
> stage nsIFile will change and the result will break us, this means we then
> have to update our code to fix the stub,

Hmm, and to fix mailnews core code too if the nsIFile interface changes. Am I wrong?

> Maybe you could post your approach of trying to set the file/directory as
> read-only, and point out where it failed, so we can take a look and see the
> issue?

I am not opposed to write the test for bug 674742 with such approach. The main subject here is that nsIFile stub is useful to write test easier, write lots of critical test cases and make slow test faster.

Unfortunately that approach has been already removed on my disk though.

Thanks.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: A new stub

I missed 'builtinclass' flag in the previous patch. After adding the flag, now the stub works fine without that fix for nsIFile.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Removing dependency.

Comment 8

[Kent James (:rkent)]

If we decide to proceed with this, let me suggest a different C++ architecture.

Conceptually what you want is a C++ object that acts just like the stock file objects, but where you can override certain classes to achieve custom behavior. This is quite similar to the challenges that I have faced in new account types.

Although in most cases I am simply using a standard C++ inheritance method, in 2010 I put together an architecture that allows you to create objects that act just like C++ xpconnect objects as far as xpcom is concerned, but can be overridden in javascript. I blogged about it here: http://mesquilla.com/2010/12/28/overriding-c-xpcom-objects-using-javascript/ though that is slightly out of date since there are minor changes I have done when I had to make it work multiplatform. But a full, current working demo of this is available here:

https://bitbucket.org/rkentjames/skinkglue

and here:

https://bitbucket.org/rkentjames/tweequilla

There are two main advantages of this over what you have proposed:

1) The C++ layer in most cases automatically includes any changes to the underlying interfaces without changes to the overridable C++ code.

2) The definition of the overridden functions can be made in javascript without having to change the C++ implementation.

Even if you do not use the msqIOverridable architecture that I do, I would encourage you to at least consider letting C++ inheritance and the NS_FORWARD_ macros do most of the work for you by creating two C++ layers.  The lower layer would create the underlying base object and forward to that using NS_FORWARD_NSIFILE, while the second layer would only override any methods that you want to test. This would be simpler and accomplish my advantage 1) but not my advantage 2)

There may be some gotchas with nsIFile that prevent this from working, but if you could do it this way you would have a much more robust implementation.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Kent, thank for the advice and the info.

I can not completely understand but __FUNCTION__ macro technique to override method is very impressive! The technique can be used in my patch.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

I am just reading a bit more... I guess that the override technique can not be used for nsIFile because nsIFile is a builtinclass. So we need a new interface (it's named nsIFileStub in my patch) as a test double for nsIFile anyway.

Comment 11

[Kent James (:rkent)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> I am just reading a bit more... I guess that the override technique can not
> be used for nsIFile because nsIFile is a builtinclass. So we need a new
> interface (it's named nsIFileStub in my patch) as a test double for nsIFile
> anyway.

Boo, that defeats a lot of the advantages. But maybe nsIFile is pretty stable anyway.

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 654917 [details] [diff] [review]
A new stub

Review of attachment 654917 [details] [diff] [review]:
-----------------------------------------------------------------

So I think I'm still not convinced by this approach, but if it is the only way, I guess we could do it.

What I would ask though is that the proxy file is implemented as a javascript test module/component if that's possible - as I think that currently these changes will insert nsIProxyLocalFile into the interfaces that we ship, and potentially ship or not the nsProxyLocalFile implementation - if it's shipped, that's unnecessarily shipping debug code, if it is not shipped, then I think it will currently break when running xpcshell tests in the packaged form.

Comment 13

[Wayne Mery (:wsmwk)]

Joshua, is this something you could pick up?

Comment 14

[Joshua Cranmer [:jcranmer]]

(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #13)
> Joshua, is this something you could pick up?

Shimming nsIFile directly is not something I'm sure I would support--especially if lower-level shims play with OS.File instead of nsIFile. Doing some sort of preload technique (as I've mentioned on the newsgroup before) is plausible, and something that I already have some experience with, although it's inherently less portable.

In either case, without specific, in-demand tests that would use this feature, I don't think it's the most valuable use of my time to push forward on this.