783414 - Make nsIFile non-builtinclass

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: fix

So we can write Test-Double of nsIFile in xpcshell test.

An example of those Test-Double is attached in bug 674742 comment #45.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Fix

I am sorry for the previous wrong patch.

Comment 2

[Benjamin Smedberg]

Comment on attachment 652638 [details] [diff] [review]
Fix

Given what I know so far about this bug, this is definitely not a good idea. We cast nsIFile* to nsLocalFile* internally in a bunch of places, and I can't understand why you'd need to mock nsIFile in a test when you could just use it directly.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Benjamin Smedberg  [:bsmedberg] [away 27-July until 7-Aug] from comment #2)
> Comment on attachment 652638 [details] [diff] [review]
> Fix
> 
> Given what I know so far about this bug, this is definitely not a good idea.
> We cast nsIFile* to nsLocalFile* internally in a bunch of places, and I
> can't understand why you'd need to mock nsIFile in a test when you could
> just use it directly.

Because I could not. How can we write test cases that nsIFile.moveTo (or other methods) fails without such mock?

Comment 4

[Benjamin Smedberg]

You make a read-only directory and try to move a file into it?

Comment 5

[Hiroyuki Ikezoe (:hiro)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> You make a read-only directory and try to move a file into it?

That's a good idea in general but can be used in the case of bug 674742 due to NS_ERROR_FILE_ACCESS_DENIED before moveTo.

I will reopen this bug when I implement another example.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Reopening.

I've post another example in bug 784872, and a new Test-Double implementation of nsIFile in bug 784870.

The new implementation can be used in most cases. In bug 784872 case, the Test-Double returns a fake size if nsIFile.fileSize is invoked.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Benjamin, what do you think of non-builtinclass only for test build (i.e. --enable-tests option for configure)? Though I do not have any idea how to do it yet.

Comment 8

[Masatoshi Kimura [:emk]]

We should test what actually shipped.
Moreover, some feature may depend on the builtinclass-ness. For example, see bug 784436.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Masatoshi Kimura [:emk] from comment #8)
> We should test what actually shipped.
> Moreover, some feature may depend on the builtinclass-ness. For example, see
> bug 784436.

Thanks for the info.

Do you have any other idea to implement test for failure cases of nsIFile method such as bug 784870?

Comment 10

[(not currently active) Ted Mielczarek]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> Benjamin, what do you think of non-builtinclass only for test build (i.e.
> --enable-tests option for configure)? Though I do not have any idea how to
> do it yet.

--enable-tests is the default, and we ship our release builds that way, FYI.

Comment 11

[Benjamin Smedberg]

In bug 784870, it appears that you're re-implementing nsIFile in C++, so why do you need builtinclass at all? Although note that still isn't safe because of internal nsIFile* -> nsLocalFile* casts, so depending on which localfile methods you call you might just crash. In any case, I do not agree with the basic premise of this bug. You'll have to find a different way to test your code.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #11)
> In bug 784870, it appears that you're re-implementing nsIFile in C++, so why
> do you need builtinclass at all? 

Woohoo!!!! Thank you, Benjamin! You reminds me to add 'builtinclass' flag to a child of nsILocalFile. Now we can write stubs of nsIFile.

Thanks a lot!