Last Comment Bug 789030 - Remove existing trailing whitespaces from our tests and shared modules
: Remove existing trailing whitespaces from our tests and shared modules
Status: RESOLVED FIXED
[mentor=davehunt][lang=js][good first...
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Pranav Ravichandran [:pranavrc]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-06 05:49 PDT by Maniac Vlad Florin (:vladmaniac)
Modified: 2013-02-18 10:58 PST (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed
fixed
fixed


Attachments
Patch for default (265.71 KB, patch)
2012-09-10 04:12 PDT, Pranav Ravichandran [:pranavrc]
hskupin: review+
Details | Diff | Review
patch for mozilla-aurora (265.73 KB, patch)
2012-09-10 06:40 PDT, Pranav Ravichandran [:pranavrc]
hskupin: review+
Details | Diff | Review
patch for mozilla-beta (263.14 KB, patch)
2012-09-10 06:41 PDT, Pranav Ravichandran [:pranavrc]
hskupin: review+
Details | Diff | Review
patch for mozilla-esr10 (251.44 KB, patch)
2012-09-10 06:42 PDT, Pranav Ravichandran [:pranavrc]
dave.hunt: review+
Details | Diff | Review
patch for mozilla-release (263.32 KB, patch)
2012-09-10 06:42 PDT, Pranav Ravichandran [:pranavrc]
dave.hunt: review+
Details | Diff | Review

Description Maniac Vlad Florin (:vladmaniac) 2012-09-06 05:49:58 PDT
We need to not only strictly avoid trailing whitespaces in our patches, but remove existent ones from our repository.
Comment 1 Alex Lakatos[:AlexLakatos] 2012-09-06 06:08:36 PDT
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] 2012-09-08 21:46:10 PDT
This command might help remove all the whitespaces in ./tests/:

> find ./tests/ -type f -exec sed -i 's/\s\+$//' {} \;
Comment 3 Henrik Skupin (:whimboo) 2012-09-09 08:49:13 PDT
(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] 2012-09-09 09:05:48 PDT
(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) 2012-09-09 09:13:31 PDT
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] 2012-09-09 10:54:46 PDT
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) 2012-09-10 01:05:19 PDT
(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] 2012-09-10 04:12:03 PDT
Created attachment 659671 [details] [diff] [review]
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) 2012-09-10 05:40:25 PDT
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) 2012-09-10 05:42:03 PDT
Landed on the default branch:
http://hg.mozilla.org/qa/mozmill-tests/rev/b57db8a3e1f6
Comment 11 Pranav Ravichandran [:pranavrc] 2012-09-10 06:40:57 PDT
Created attachment 659689 [details] [diff] [review]
patch for mozilla-aurora
Comment 12 Pranav Ravichandran [:pranavrc] 2012-09-10 06:41:40 PDT
Created attachment 659690 [details] [diff] [review]
patch for mozilla-beta
Comment 13 Pranav Ravichandran [:pranavrc] 2012-09-10 06:42:17 PDT
Created attachment 659691 [details] [diff] [review]
patch for mozilla-esr10
Comment 14 Pranav Ravichandran [:pranavrc] 2012-09-10 06:42:58 PDT
Created attachment 659692 [details] [diff] [review]
patch for mozilla-release
Comment 15 Henrik Skupin (:whimboo) 2012-09-10 11:48:41 PDT
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) 2012-09-10 11:50:31 PDT
Landed on Aurora:
http://hg.mozilla.org/qa/mozmill-tests/rev/b57db8a3e1f6
Comment 17 Henrik Skupin (:whimboo) 2012-09-10 11:57:11 PDT
(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) 2012-09-10 12:00:25 PDT
Landed on beta:
http://hg.mozilla.org/qa/mozmill-tests/rev/211f23daebd9
Comment 19 Dave Hunt (:davehunt) 2012-09-11 01:27:24 PDT
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) 2012-09-11 01:36:17 PDT
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) 2012-09-11 01:37:33 PDT
Thanks Pranav. Let us know if you're interested in contributing anything else! :)
Comment 22 Pranav Ravichandran [:pranavrc] 2012-09-11 04:26:28 PDT
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) 2012-09-11 04:30:07 PDT
(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) 2013-02-18 10:58:32 PST
(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/[ ]*$//'

Note You need to log in before you can comment on or make changes to this bug.