295205 - runtests.pl should check that the line endings of files conform to your OS standard

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Marc Schumann [:Wurblzap]]

I see trailing ^M characters in buglist.cgi line 844, summarize_time.cgi line
325 and Bugzilla/User.pm lines 601 and 652.

Comment 1

[Marc Schumann [:Wurblzap]]

Attachment: Patch

I needed to hack the patch file because Windows (?) mangled the line breaks --
I hope it still works out.

Comment 2

[Frédéric Buclin]

Comment on attachment 184305 [details] [diff] [review]
Patch

I did

find . -type f -exec dos2unix -UTb {} \;

and then a cvs diff. I found all the lines mentioned in your patch but with
some additional ones in request.cgi, line 108; FlagType.pm, line 518 and
template/en/default/list/list.html.tmpl, lines 55-xx.

This patch makes sense only if a ^M detection is set in runtests.pl, e.g. in
005no_tabs.t. Else you could reopen this bug every week or so. ;)

Comment 3

[Frédéric Buclin]

Per discussion with Marc and Colin, we came to the conclusion that instead of
detecting non-UNIX line ending, even under Windows, runtests.pl should make sure
that the line ending in files is conform to your OS. The conversion to UNIX
format will be done by cvs when commiting files to the server.

I still think that 005no_tabs.t is a good place for this feature.

Comment 4

[Colin Ogilvie [:cso]]

Attachment: Test for Windows chars

Not sure if this actually works successfully on windows or not, but I'll put it
here anyway.

Comment 5

[Colin Ogilvie [:cso]]

Attachment: Test for Windows chars

Again, not sure if this works on Windows... will test soon.

Comment 6

[Colin Ogilvie [:cso]]

Seems to work on Windows too...

Comment 7

[Colin Ogilvie [:cso]]

And yet again, I get bitten by the "Reassign bug to owner and QA contact of
selected component" being too close to commit.

Comment 8

[Marc Schumann [:Wurblzap]]

In agreement with comment 3, I think we should put this into 005no_tabs.t and
call it 005whitespace.t.

Comment 9

[Colin Ogilvie [:cso]]

I couldn't convince it to work in 005_notabs.t which is why it changed to
010_nowin.t

Comment 10

[Marc Schumann [:Wurblzap]]

Attachment: Patch, replacing 005no_tabs.t by 005whitespace.t

Tested on Windows.

Comment 11

[Frédéric Buclin]

Comment on attachment 189786 [details] [diff] [review]
Patch, replacing 005no_tabs.t by 005whitespace.t

>+#################
>+#Bugzilla Test 5#
>+###whitespace####

005whitespace is not more appropriate than 005no_tabs, because line endings has
nothing to do with whitespaces. What about something like 005line_check ? or
005line_compliance ?


>+foreach my $file (@testitems) {
>+    open (FILE, "$file");
>+    my @contents = <FILE>;
>+    if (grep /\t/, @contents) {
>+        ok(0, "$file contains tabs --WARNING");
>+    } else {
>+        ok(1, "$file has no tabs");
>+    }
>+    if (grep /\r/, @contents) {
>+        ok(0, "$file contains non-OS-conformant line endings --WARNING");
>+    } else {
>+        ok(1, "All line endings of $file are OS conformant");
>+    }
>+    close (FILE);
>+}

This loop must be split into two distinct loops, see e.g. 002goodperl.t, where
each set of tests has its own loop. This way, we have first all tests about
tabs, and then all tests about line endings; instead of a mix of both.

I tested your patch on my installation and it works fine. Marc, have you tested
it on Windows?

Comment 12

[Frédéric Buclin]

Comment on attachment 189757 [details]
Test for Windows chars

Looks better to move this test into 005no_tabs.

Comment 13

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.2

Addressing comments. Works on Windows and need *nix and Mac testing. For some reason, on Windows, it doesn't complain about files which are *completely* \n-formatted (instead of \r\n). This may be because of some Cygwin magic. It does catch single offending lines, though.

I really do think the name should be 005whitespace instead of 005no_tabs. Line breaks are whitespace, not tabs.

Comment 14

[Frédéric Buclin]

Comment on attachment 201661 [details] [diff] [review]
Patch 1.2

>Index: t/005no_tabs.t

> foreach my $file (@testitems) {
>     open (FILE, "$file");
>-    if (grep /\t/, <FILE>) {
>-        ok(0, "$file contains tabs --WARNING");
>+    my @contents = <FILE>;
>+    if (grep /\t/, @contents) {
>+        push(@{$results{'tabs'}}, [$file, 0]);
>+    } else {
>+        push(@{$results{'tabs'}}, [$file, 1]);
>+    }
>+    if (grep /\r/, @contents) {
>+        push(@{$results{'linebreaks'}}, [$file, 0]);
>     } else {
>-        ok(1, "$file has no tabs");
>+        push(@{$results{'linebreaks'}}, [$file, 1]);
>     }
>     close (FILE);
> }
> 
>+foreach my $category (keys(%msg)) {
>+    foreach (@{$results{$category}}) {
>+        my ($file, $result) = @$_;
>+        ok($result, sprintf($msg{$category}{$result}, $file));
>+    }
>+}


Even in this case, you have two FOREACH loops (3 even). All other *.t tests display results in real time, i.e. before analyzing the next file. You should do the same here and have two distinct loops, one for tabs, and a second one for line ending. Else it looks good.

Comment 15

[Frédéric Buclin]

The trunk is now frozen to prepare Bugzilla 2.22. Enhancement bugs are retargetted to 2.24.

Comment 16

[Frédéric Buclin]

We are freezing the code for 3.0 in two weeks and we don't expect this bug to be fixed on time.

Comment 17

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.3

This hasn't lain dormant for even three years yet, but I think it's time to pick it up anyway.

Separated loops now.

Comment 18

[Frédéric Buclin]

Comment on attachment 340614 [details] [diff] [review]
Patch 1.3

Seems to work. But on Windows, it didn't complain about files entirely using *nix line endings, as Marc already reported. As I don't use Cygwin on Windows, that's unrelated to it. On the opposite direction, Linux immediately complains when files have Windows line endings.

r=LpSolit for Linux (and on Windows, but noting that it doesn't complain very often). We still have to test it on Mac. justdave?

Comment 19

[Max Kanat-Alexander]

Comment on attachment 340614 [details] [diff] [review]
Patch 1.3

This is fine. Checking for correct Windows line endings is too much effort for too little benefit, I think.

Comment 20

[Max Kanat-Alexander]

I don't see any reason not to backport this to at least 3.2.

Comment 21

[Frédéric Buclin]

Max, I prefer not to rename a file on a stable branch, even for a test file. This patch is not critical enough to worth the change. Let's rediscuss if you disagree, but marking as approval3.2- for now.

Comment 22

[Frédéric Buclin]

Retargetting to 3.4 per our discussion on IRC.

Comment 23

[Marc Schumann [:Wurblzap]]

Removing t/005no_tabs.t;
/cvsroot/mozilla/webtools/bugzilla/t/005no_tabs.t,v  <--  005no_tabs.t
new revision: delete; previous revision: 1.13
done
Checking in t/005whitespace.t;
/cvsroot/mozilla/webtools/bugzilla/t/005whitespace.t,v  <--  005whitespace.t
new revision: 1.14; previous revision: 1.13
done