Closed Bug 142744 Opened 22 years ago Closed 20 years ago

Testing suite should work on Win32

Categories

(Bugzilla :: Testing Suite, enhancement)

2.15
x86
Windows 2000
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: jouni, Assigned: glob)

References

Details

Attachments

(1 file, 2 obsolete files)

Windows development is seriously harmed by the fact that the testing suite
doesn't run on Windows. I can take a look at this as I'm the one with the need. 

Ccing zach@zachlipton.com for comments: do you know if this is downright
impossible? At least the runtests.sh needs a Windows companion, but wonder if
Test:: modules work equally on Win32 platforms?
The Test::* should work find under win32, but there are two issues that I 
know of:

1. runtests.sh, we should rewrite it in perl so it works anywhere
2. Filepaths, I think we currently assume that the directory seperator is a '/' 
and not a '\', this needs to be fixed to use this little code nugget from my 
bucket-o-tricks:

use File::Spec;
sub fixpath($) {
    my ($path) = @_;
    my @pathlist = split('/',$path);
    return File::Spec->catfile(@pathlist);
}

This takes a path using a standard '/' directory seperator and returns it for 
the current filesystem. 
Ok, thanks for your input. I'll delve into this soonish. Marking 2.18 per the
"Out-of-the-box compatibility with Win32" statement in the Bugzilla roadmap.
Target Milestone: --- → Bugzilla 2.18
Depends on: 143155
I've cast a quick glance at the code. The essential problems seem to be the some
instances of backticks and input redirection - that stuff is fairly unixy. It's
probably going to take a while before this truly works, but I'm looking into it. 

ActiveState is doing quite an impressive job hiding the problems with \ and /
conflicts, by the way...
Attached patch Patch v1 (obsolete) — Splinter Review
This should make it tick...
Ok, now the version version is ready for review. In order to test this on Win32
you also need the new runtests.pl from bug 143155; unix tests can be run with
the old runtests.sh.

I've tested the patch on Win32 ActiveState Perl 5.6.1 and on Perl 5.6.0 on
netbsd. My other Unix testing platforms are currently in pieces, so can't test
more on those. The changes should be safe though.

Ccing justdave since all the more appropriate reviewers are unavailable for a
while and you do know something about Win32 (according to some comments on bug
124174).
Status: NEW → ASSIGNED
Keywords: patch, review
Comment on attachment 84078 [details] [diff] [review]
Patch v1

This is really really ugly, but if it works for you, r=bbaetz x2

I suspect using IPC::Run for the test suite would be overkill.
Attachment #84078 - Flags: review+
Questions: Do we need to put win32Exec() in every test file? Why not put it in a
module rather than using the same block of code 4+ times? Also, can we have a
bugzillaexec() sub that Does The Right Thing for windows and for linux instead
of things like my $loginfo = $iswin ? win32Exec($command) : `$command 2>&1`;
Actually, why don't we just slurp the file into a var, wrap it in a sub so that
code isn't run, and then eval it?

Thats what mod_perl does, and we'll have to do that to test suitablilty. The
downside is that thta will generate warnings at the moment, because our code
isn't designed to work that way
Comment on attachment 84078 [details] [diff] [review]
Patch v1

This is bitrotten, so obsoleting.

Zach, Bbaetz: Some of what you're discussing implies changes of a wider scope
for the testing suite. That's way beyond my resources now. If you consider this
approach useful, I might try refreshing the patch. What do you think about
this?
Attachment #84078 - Attachment is obsolete: true
Umh. Do we consider this to be a 2.18 blocker?

I'd go for saying no, since that would mostly be catering for me, myself and
couple of other people who develop on Windows. "Out of the box compatibility"
maybe shouldn't be extended to development tools.

This patch should be almost totally rewritten, as the testing suite has changed
quite a bit since I wrote this patch 18 months ago...
It'd be nice, but not necessary. I agree.

On the other hand, once upon a time, Jake had a tinderbox running on a Win32
box...  and it worked.  Did something break since then?
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
Attached patch 008filter.t patch (obsolete) — Splinter Review
all tests work ok for me on win32, with the exception of 008filter, due to the
path separator.

here's a patch.
Attachment #150329 - Flags: review?(jouni)
Comment on attachment 150329 [details] [diff] [review]
008filter.t patch

Sorry it's taken so long, I've been insanely busy for the last weeks.

>+        if ($^O eq 'MSWin32') {
>+          # filterexceptions.pl uses / so convert to \ on win32

Make this comment something like 

# filterexceptions.pl uses / separated paths, while 
# find_actual_files returns \ separated ones on Windows.
# Here, we convert the filter exception hash to use \.

>+          foreach my $file (keys %safe) {
>+            my $orig_file = $file;
>+            $file =~ s|/|\\|g;
>+            $safe{$file} = $safe{$orig_file};

Throw in a

$safe{$orig_file} = undef unless $file eq $orig_file;

for good measure; it's not necessary to store duplicate entries with both path
separators. Although that's hardly relevant in any practical sense, it's also a
matter of clarity; we do want to _alter_, not duplicate the hash keys.

Other than those, r=jouni. I'm surprised rest of the testing suite worked so
painlessly. That's good. :)
Attachment #150329 - Flags: review?(jouni) → review+
Assignee: jouni → bugzilla
Status: ASSIGNED → NEW
Flags: approval?
Status: NEW → ASSIGNED
addresses jouni's comments
Attachment #150329 - Attachment is obsolete: true
Flags: approval? → approval+
Target Milestone: Bugzilla 2.20 → Bugzilla 2.18
can someone please check this patch in for me.
Certainly.

Checking in 008filter.t;
/cvsroot/mozilla/webtools/bugzilla/t/008filter.t,v  <--  008filter.t
new revision: 1.12; previous revision: 1.11
done
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: