Randy Kobes wrote:
On Wed, 1 Oct 2003, Geoffrey Young wrote:


or, I could just commit it now and Randy can decide which
route to go.  I think I'll just do that...


Here's a revised set of tests, using Geoff's implementation
of Apache::CRLF. This also addresses a couple of earlier
comments of Stas - the files used for comparison are now
assumed to be found as t/htdocs/perlio/http.pod and
t/htdocs/perlio/http_cycle.png, and also a constant
data file name is used (and then cleaned up after the
tests are done).

Cool. The following are just style comments.


+    # test reading and writing text and binary files
+    {
+        local $/;

Randy, can you please put local $/; just before it's needed? Otherwise it's too far away from its usage and it may affect other things if the test is expanded in the future. The best practice IMHO is:


           {
              local $/;
              ... <$fh>
           }

in every place you need it. this makes the code easier to read.

+ my ($rfh, $wfh, $pfh);

why these are defined outside and not where they are opened?


open my $rfh, "<:APR", $in, $r->pool

etc.

+        for my $file ('http.pod', 'http_cycle.png') {
+            my $in = catfile $dir, $file;
+            my $out = catfile $dir, "$file.out";
+            open $rfh, "<:APR", $in, $r->pool
+                or die "Cannot open $in for reading: $!";
+            binmode($rfh);  # not necessary

if it's not necessary, why do we need it?


+            my $apr_content = <$rfh>;
+            close $rfh;
+            open $pfh, "<", $in
+                or die "Cannot open $in for reading: $!";
+            binmode($pfh);
+            my $perl_content = <$pfh>;
+            close $pfh;
+            ok t_cmp(length $perl_content,
+                     length $apr_content,
+                     "testing data size of $file");
+
+            open $wfh, ">:APR", $out, $r->pool
+                or die "Cannot open $out for writing: $!";
+            print $wfh $apr_content;
+            close $wfh;
+            ok t_cmp(-s $in,
+                     -s $out,
+                     "testing file size of $file");
+            unlink $out;
+        }

I'd enclose the following code of its own block showing that it's an independent sub-test. you won't need to predeclare lexicals vars above. This practice makes easier to debug subtests where you can comment out blocks without affecting the rest of the test. see apr/pool.pm for such an example.


+        my $scratch = catfile $dir, 'scratch.dat';
+        my $text;
+        my $count = 2000;
+        open $wfh, ">:crlf", $scratch
+            or die "Cannot open $scratch for writing: $!";
+        print $wfh 'a' . ((('a' x 14) . "\n") x $count);
+        close $wfh;
+        open $rfh, "<:APR", $scratch, $r->pool
+            or die "Cannot open $scratch for reading: $!";
+        $text = <$rfh>;
+        close $rfh;
+        ok t_cmp($count,
+                 count_chars($text, Apache::CRLF),
+                 'testing for presence of \015\012');
+        ok t_cmp($count,
+                 count_chars($text, "\n"),
+                 'testing for presence of \n');
+
+        open $wfh, ">:APR", $scratch, $r->pool
+            or die "Cannot open $scratch for writing: $!";
+        binmode($wfh);  # not necessary
+        print $wfh 'a' . ((('a' x 14) . Apache::CRLF) x $count);
+        close $wfh;
+        open $rfh, "<:APR", $scratch, $r->pool
+            or die "Cannot open $scratch for reading: $!";
+        $text = <$rfh>;
+        close $rfh;
+        ok t_cmp($count,
+                 count_chars($text, Apache::CRLF),
+                 'testing for presence of \015\012');
+        ok t_cmp($count,
+                 count_chars($text, "\n"),
+                 'testing for presence of \n');
+        open $rfh, "<:crlf", $scratch
+            or die "Cannot open $scratch for reading: $!";
+        $text = <$rfh>;
+        close $rfh;
+        ok t_cmp(0,
+                 count_chars($text, Apache::CRLF),
+                 'testing for presence of \015\012');
+        ok t_cmp($count,
+                 count_chars($text, "\n"),
+                 'testing for presence of \n');
+
+        my $utf8 = "\x{042F} \x{0432}\x{0430}\x{0441} \x{043B}\x{044E}";
+        open $wfh, ">:APR", $scratch, $r->pool
+            or die "Cannot open $scratch for writing: $!";
+        binmode($wfh, ':utf8');
+        print $wfh $utf8;
+        close $wfh;
+        open $rfh, "<:APR", $scratch, $r->pool
+            or die "Cannot open $scratch for reading: $!";
+        binmode($rfh, ':utf8');
+        $text = <$rfh>;
+        close $rfh;
+        ok t_cmp($utf8,
+                 $text,
+                 'utf8 binmode test');
+        unlink $scratch;
+    }

     # XXX: need tests
     # - for stdin/out/err as they are handled specially
@@ -232,6 +323,13 @@
     # cleanup: t_mkdir will remove the whole tree including the file

     Apache::OK;
+}
+
+sub count_chars {
+    my($text, $chars) = @_;
+    my $seen = 0;
+    $seen++ while $text =~ /$chars/g;
+    return $seen;
 }

1;

===============================================================



--


__________________________________________________________________ Stas Bekman JAm_pH ------> Just Another mod_perl Hacker http://stason.org/ mod_perl Guide ---> http://perl.apache.org mailto:[EMAIL PROTECTED] http://use.perl.org http://apacheweek.com http://modperlbook.org http://apache.org http://ticketmaster.com


--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]



Reply via email to