789030 - Remove existing trailing whitespaces from our tests and shared modules

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Maniac Vlad Florin (:vladmaniac)]

We need to not only strictly avoid trailing whitespaces in our patches, but remove existent ones from our repository.

Comment 1

[Alex Lakatos[:AlexLakatos]]

A good start would be to run this in mozmill-tests
>grep -r ' $' ./
That will give you a list with all occurences.

>grep -r ' $' ./tests/ | cut -f1 -d : | uniq
That will give you a list with just the file paths, and after that it's a matter of opening them and having fun.

Comment 2

[Pranav Ravichandran [:pranavrc]]

This command might help remove all the whitespaces in ./tests/:

> find ./tests/ -type f -exec sed -i 's/\s\+$//' {} \;

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Pranav Ravichandran [:pranavrc] from comment #2)
> This command might help remove all the whitespaces in ./tests/:
> 
> > find ./tests/ -type f -exec sed -i 's/\s\+$//' {} \;

It might very well do that. Would you mind to work on a patch?

Comment 4

[Pranav Ravichandran [:pranavrc]]

(In reply to Henrik Skupin (:whimboo) from comment #3)
> (In reply to Pranav Ravichandran [:pranavrc] from comment #2)
> > This command might help remove all the whitespaces in ./tests/:
> > 
> > > find ./tests/ -type f -exec sed -i 's/\s\+$//' {} \;
> 
> It might very well do that. Would you mind to work on a patch?

Sure! I just thought taking up good first bugs beyond the first couple of patches wasn't allowed. So, should I run this from m-c root or just *test*/ directories?

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Great! The tests we are referring to here are not located in m-c but in a separate repository which you can find here: http://hg.mozilla.org/qa/mozmill-tests/

This repository contains a couple of named branches and we would have to run this command for each of those. See the following URL for details:

https://developer.mozilla.org/en-US/docs/Mozmill_Tests#Handling_branches

Comment 6

[Pranav Ravichandran [:pranavrc]]

I've removed the whitespaces, using this at the root of mozmill-tests (just in case the command is needed for review):

> find . -not \( -name .hg -prune \) -type f | xargs file | grep -i  ASCII |cut -d: -f1| xargs sed 's/\s\+$//' -i

I have a bunch of patches, one for each branch now. Is it required that way, or is there some other way to generate a patch/diff for the whole repository, inclusive of all branches?

Comment 7

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Pranav Ravichandran [:pranavrc] from comment #6)
> I've removed the whitespaces, using this at the root of mozmill-tests (just
> in case the command is needed for review):
> 
> > find . -not \( -name .hg -prune \) -type f | xargs file | grep -i  ASCII |cut -d: -f1| xargs sed 's/\s\+$//' -i
> 
> I have a bunch of patches, one for each branch now. Is it required that way,
> or is there some other way to generate a patch/diff for the whole
> repository, inclusive of all branches?

We are having a single patch for the default branch at first, that is Firefox Nightly. 
If that is approved and checked in, then we handle other branches. As a process, this can be both transplating the default patch if applies, or build other patches separately if needed. 
This bug would definitely need separate patches for branches because its a refactoring fix and handles multiple files. 

Does that make any sense to you? Please come back and I'll be happy to give more details

Comment 8

[Pranav Ravichandran [:pranavrc]]

Attachment: Patch for default

Ah right, if default passes, you hg graft it to other branches or separately patch the other branches. Thanks! Here's the patch for the default branch.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 659671 [details] [diff] [review]
Patch for default

Review of attachment 659671 [details] [diff] [review]:
-----------------------------------------------------------------

Wow. I have never thought that we have so many trailing white-space issues. This patch looks great and I'm going to land it right now. Can you please check if it can be cleanly applied to the other branches? If not mind giving us appropriate backport patches? Thanks!

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed on the default branch:
http://hg.mozilla.org/qa/mozmill-tests/rev/b57db8a3e1f6

Comment 11

[Pranav Ravichandran [:pranavrc]]

Attachment: patch for mozilla-aurora

Comment 12

[Pranav Ravichandran [:pranavrc]]

Attachment: patch for mozilla-beta

Comment 13

[Pranav Ravichandran [:pranavrc]]

Attachment: patch for mozilla-esr10

Comment 14

[Pranav Ravichandran [:pranavrc]]

Attachment: patch for mozilla-release

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 659689 [details] [diff] [review]
patch for mozilla-aurora

Review of attachment 659689 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! Next time please take care of the review flags so that we are made aware of a review request by bugzilla.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed on Aurora:
http://hg.mozilla.org/qa/mozmill-tests/rev/b57db8a3e1f6

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) from comment #16)
> Landed on Aurora:
> http://hg.mozilla.org/qa/mozmill-tests/rev/b57db8a3e1f6

That was the wrong link. Here the updated one:
http://hg.mozilla.org/qa/mozmill-tests/rev/c4adf71bd262

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed on beta:
http://hg.mozilla.org/qa/mozmill-tests/rev/211f23daebd9

Comment 19

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 659692 [details] [diff] [review]
patch for mozilla-release

Review of attachment 659692 [details] [diff] [review]:
-----------------------------------------------------------------

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/8c41e9647e9a (release)

Comment 20

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 659691 [details] [diff] [review]
patch for mozilla-esr10

Review of attachment 659691 [details] [diff] [review]:
-----------------------------------------------------------------

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/a510104d7c17 (esr10)

Comment 21

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Thanks Pranav. Let us know if you're interested in contributing anything else! :)

Comment 22

[Pranav Ravichandran [:pranavrc]]

Absolutely! Do give me a heads up if there's anything I could work on, and meanwhile, I'll be lurking on bugsahoy and the bugzilla search trying to find something.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Pranav Ravichandran [:pranavrc] from comment #22)
> Absolutely! Do give me a heads up if there's anything I could work on, and
> meanwhile, I'll be lurking on bugsahoy and the bugzilla search trying to
> find something.

Pranav, it would be fantastic if you could join our #automation channel on IRC so that we can figure out which type of bugs would be best for you. I want to be sure to direct you into the direction of your own interest. If you aren't able to join let us know and we can follow-up via email.

Comment 24

[Jonathan French (:jfrench)]

(In reply to Pranav Ravichandran [:pranavrc] from comment #6)
> I've removed the whitespaces, using this at the root of mozmill-tests (just
> in case the command is needed for review):
> 
> > find . -not \( -name .hg -prune \) -type f | xargs file | grep -i  ASCII |cut -d: -f1| xargs sed 's/\s\+$//' -i
> 

Interesting, I was not able to purge trailing end of line whitespace using the mozilla-build shell on windows, with the sed expression described. 

> sed 's/\s\+$//'

-i was also not a recognized option to sed on the mozilla-build shell. Maybe this was unique to the shell or OS being used.

I had do do this below to reliably remove trailing end of line white space (excluding unwanted 'tab'whitespace, which is a different thing altogether).

sed 's/[ ]*$//'