Remove existing trailing whitespaces from our tests and shared modules

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vladmaniac, Assigned: pranavrc)

Tracking

unspecified

Firefox Tracking Flags

(firefox15 fixed, firefox16 fixed, firefox17 fixed, firefox18 fixed, firefox-esr10 fixed)

Details

(Whiteboard: [mentor=davehunt][lang=js][good first bug])

Attachments

(5 attachments)

(Reporter)

Description

5 years ago
We need to not only strictly avoid trailing whitespaces in our patches, but remove existent ones from our repository.
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.
Whiteboard: [mentor=davehunt][good first bug]
Whiteboard: [mentor=davehunt][good first bug] → [mentor=davehunt][lang=js][good first bug]
(Assignee)

Comment 2

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

> find ./tests/ -type f -exec sed -i 's/\s\+$//' {} \;
(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?
(Assignee)

Comment 4

5 years ago
(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?
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
Assignee: nobody → prp.1111
Status: NEW → ASSIGNED
(Assignee)

Comment 6

5 years ago
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?
(Reporter)

Comment 7

5 years ago
(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
(Assignee)

Comment 8

5 years ago
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.
Attachment #659671 - Flags: feedback?(dave.hunt)
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!
Attachment #659671 - Flags: feedback?(dave.hunt) → review+
Landed on the default branch:
http://hg.mozilla.org/qa/mozmill-tests/rev/b57db8a3e1f6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox-esr10: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → fixed
Resolution: --- → FIXED
(Assignee)

Comment 11

5 years ago
Created attachment 659689 [details] [diff] [review]
patch for mozilla-aurora
(Assignee)

Comment 12

5 years ago
Created attachment 659690 [details] [diff] [review]
patch for mozilla-beta
(Assignee)

Comment 13

5 years ago
Created attachment 659691 [details] [diff] [review]
patch for mozilla-esr10
(Assignee)

Comment 14

5 years ago
Created attachment 659692 [details] [diff] [review]
patch for mozilla-release
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.
Attachment #659689 - Flags: review+
Landed on Aurora:
http://hg.mozilla.org/qa/mozmill-tests/rev/b57db8a3e1f6
status-firefox17: affected → fixed
Attachment #659690 - Flags: review?(hskupin)
Attachment #659691 - Flags: review?(dave.hunt)
Attachment #659692 - Flags: review?(dave.hunt)
Attachment #659690 - Flags: review?(hskupin) → review+
(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
Landed on beta:
http://hg.mozilla.org/qa/mozmill-tests/rev/211f23daebd9
status-firefox16: affected → fixed
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)
Attachment #659692 - Flags: review?(dave.hunt) → review+
status-firefox15: affected → fixed
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)
Attachment #659691 - Flags: review?(dave.hunt) → review+
status-firefox-esr10: affected → fixed
Thanks Pranav. Let us know if you're interested in contributing anything else! :)
(Assignee)

Comment 22

5 years ago
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.
(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.
(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/[ ]*$//'
You need to log in before you can comment on or make changes to this bug.