483085 - 012throwables.t does not examine all .pm files

Status: RESOLVED FIXED

(Bugzilla :: Testing Suite, defect)

Description

[David Marshall]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.7) Gecko/2009021906 Firefox/3.0.7
Build Identifier: Bugzilla 3.2.2

If, for instance, Net::LDAP is not installed, the list @Support::Files::testitems will not include Bugzilla/Auth/Verify/LDAP.pm.  However, this file contains the uses of throwables such as ldap_bind_failed.  So, t/012throwables will report a number of items as existing that aren't thrown.

Reproducible: Always

Steps to Reproduce:
1. Ensure that Net::LDAP is not installed
2. Run t/012throwables.t

Actual Results:  
# code error tag 'ldap_bind_failed' is defined at line(s) (274) but is not used anywhere


Expected Results:  
error tag ldap_bind_failed not listed as an error

Comment 1

[David Marshall]

Attachment: v1

Comment 2

[David Marshall]

Comment on attachment 367124 [details] [diff] [review]
v1

LpSolit pointed out that this would wind up checking undesired files.

Comment 3

[David Marshall]

Attachment: v2

LpSolit pointed out that files get bumped out of @Support::Files::testitems only because 001compile.t isn't going to be able to compile them.  So, idea of patch is to create a hash of excluded files that 001compile.t can use to skip files.

Comment 4

[Gervase Markham [:gerv]]

Comment on attachment 367131 [details] [diff] [review]
v2

So now you have the situation where there's a list of "exclude files" in Support::Files, but those files are actually only excluded from test 001.

Either make it so that isTestingFile() excludes all the exclude files (i.e. it's a proper exclude list for everything), or move the creation and storage of the list entirely into text 001 (if the list is specific to test 001). Let's not have this half-way thing :-)

Gerv

Comment 5

[Frédéric Buclin]

(In reply to comment #4)
> So now you have the situation where there's a list of "exclude files" in
> Support::Files, but those files are actually only excluded from test 001.

Yes, and that's the correct behavior. There is no reason to exclude these files from other test scripts. The only reason we exclude them in 001compile.t is to not fail when a required package is missing. In all other cases, that's not a problem.


> Either make it so that isTestingFile() excludes all the exclude files (i.e.
> it's a proper exclude list for everything)

No, that's not the job of isTestingFile() IMO. Excluding files by default is the wrong way to go. The patch removes this check on purpose.



, or move the creation and storage of
> the list entirely into text 001 (if the list is specific to test 001).

This would mean duplicating a lot of code, which is suboptimal. And as I said above, excluding files by default is the wrong behavior. So I'm totally fine with this patch.

Comment 6

[David Marshall]

Note that 001 is already deciding whether to attempt compilation of modules based on which DBD modules are installed, so shouldn't it be the case that 001 either makes all decisions about exclusions or none?

I'm leaning towards moving %exclude_deps into 001, because at this time, it's the only test that cares about its contents.  But I'm amenable to whatever Frédéric and Gervase can agree upon.

Comment 7

[Gervase Markham [:gerv]]

Frederik: why does moving the list into test 001 lead to duplicating a lot of code? If (as Dave says), it's the only test that cares about the exclude list and, once the patch is in, it'll also be the only code file checking the list, then I can't see where there would be duplication.

I still think this is the right course of action.

Gerv

Comment 8

[Frédéric Buclin]

(In reply to comment #7)
> Frederik: why does moving the list into test 001 lead to duplicating a lot of
> code?

It would lead to code duplication if isTestingFile() excludes some files and the exclusion list is in 001. If we remove this check from isTestingFile() as done in the current patch, and we move the exclusion list into 001, then yes, we would avoid code duplication and this would work fine. If that's what you meant, then we both agree.

Comment 9

[Frédéric Buclin]

Fixed by bug 527505.