839705 - Clean up tests in all branches with trailing blank lines

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P5)

Description

[Jonathan French (:jfrench)]

Attachment: tests with trailing blank lines (default)

Tasks begat from Comments 87 through 92 in Bug 789916:
https://bugzilla.mozilla.org/show_bug.cgi?id=789916#c87

(In reply to Henrik Skupin (:whimboo) from comment #87)
> No, there is a difference between 'newline character' and 'blank line'.

1) Update the Mozmill Style Guide doc to better clarify what a newline character is and provide a code example
https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Mozmill_Style_Guide#Whitespace

It is probably the best and clearest way for folks to not get confused about what is being asked for in the style guide. I looked at it, in conjunction with several examples I hit across the repo with 'blank lines' as the last line of the test, and interpreted in conjunction with the style guide(incorrectly) as a new 'empty' line as the desired format.

2) Correct the numerous files across all branches that have incorrect empty blank lines at their end of file.

I did a scan across the default branch with a simple xargs tail, and came across 33 files in default that are being incorrectly terminated with a blank line. I have attached the list for default.

If you would like it addressed, I would be happy to do the clean up.

Comment 1

[Jonathan French (:jfrench)]

As for part 2) - I've built a patch for the default branch, in advance of the bug being evaluated or assigned, if you want to fix it. I will attach it shortly.

I've recursively checked and compared default, aurora, beta, release, esr17, and there are many similarities; but enough differences such that a separate patch will be required for most, if not all branches.

Comment 2

[Jonathan French (:jfrench)]

Attachment: clean up trailing blank lines (default)

Patch "clean up trailing blank lines (default)" for the default branch. Thirty-three files affected.

Tested with Default Nightly 21.0a1 20130211031055. Tests pass where expected.

Comment 3

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

Comment on attachment 712630 [details] [diff] [review]
clean up trailing blank lines (default)

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

This looks good, but I don't believe it is complete. I also found trailing blank lines in the following files:

data/popups/popup_trigger.html
data/security/enable_privilege.html
tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js
tests/endurance/testTabbedBrowsing_OpenNewWindow/test1.js
tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js

Comment 4

[Jonathan French (:jfrench)]

Confirmed the first two need fixing
tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js
tests/endurance/testTabbedBrowsing_OpenNewWindow/test1.js

This last though, appears to have content on its last line with no trailing whitespace - it seems correct to me
tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js

I will re-check the data dir. I didn't realize .html files in /data were also of interest.

Comment 5

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

(In reply to Jonathan French (:jfrench) from comment #4)
> This last though, appears to have content on its last line with no trailing
> whitespace - it seems correct to me
> tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js

That's odd - I can see it locally, but not on hg.mozilla.org...

Comment 6

[Jonathan French (:jfrench)]

Attachment: clean up trailing blank lines (default)

Patch "clean up trailing blank lines (default)" for the default branch. Thirty-seven files modified.

Two additional test files were modified and added to the patch, tested with Default Nightly 21.0a1 20130212031120. Tests pass where expected.

Two additional html files were modified and added to the patch, loaded into Default Nightly 21.0a1 20130212031120. Files load as expected.

Also re-checked the default branch, found no other files requiring modification at present.

Comment 7

[Jonathan French (:jfrench)]

Oh yes, there is one manifest file that shows up as 'blank' in the search; but it is a completely empty file, so I assume we leave that one as is.

tests/crash/manifest.ini

Comment 8

[Jonathan French (:jfrench)]

I just re-checked
tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js

somebody must have returned a change to it since I built the list for default, so it does indeed now have an incorrect blank-line end of file. I will build a new default patch, including a fix for it.

Comment 9

[Jonathan French (:jfrench)]

Wow files are being "blank lined" in the repo by the hour, it seems. I just did another pull a few minutes ago from default and another one was just padded with an eof blank line.

tests\endurance\testTabView_SwitchTabs\test1.js 

I've corrected that one also. 

The total for default patch will be 39 files modified.

Comment 10

[Jonathan French (:jfrench)]

Attachment: clean up trailing blank lines (default)

Patch "clean up trailing blank lines (default)" for the default branch. Thirty-nine files modified.

Newly added files in the patch, tested with Default Nightly 21.0a1 20130213031137. Tests pass where expected.

Comment 11

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

Comment on attachment 713578 [details] [diff] [review]
clean up trailing blank lines (default)

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

Looks great, thanks Jonathan.

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/06ce35a5fa41

Comment 12

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

(In reply to Jonathan French (:jfrench) from comment #9)
> Wow files are being "blank lined" in the repo by the hour, it seems. I just
> did another pull a few minutes ago from default and another one was just
> padded with an eof blank line.

Unfortunately these are easy to miss in a review. I'll send a note to the mailing list warning reviewers to look out for this style guide infraction. To be honest, I've probably let most of these through! :)

Comment 13

[Jonathan French (:jfrench)]

Aurora patch coming up momentarily.

Comment 14

[Jonathan French (:jfrench)]

Attachment: clean up trailing blank lines (aurora)

Patch "clean up trailing blank lines (aurora)" for the aurora branch. Thirty-seven files modified.

Tested with Aurora Nightly 20.0a2 20130214042018. Tests pass where expected.

Comment 15

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

Comment on attachment 714071 [details] [diff] [review]
clean up trailing blank lines (aurora)

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/15571c40330c

Comment 16

[Jonathan French (:jfrench)]

Beta patch arriving momentarily.

Comment 17

[Jonathan French (:jfrench)]

Attachment: clean up trailing blank lines (beta)

Patch "clean up trailing blank lines (beta)" for the beta branch. Thirty-six files modified. Each branch has a different set of files affected, hence separate patches.

Tested with Beta 19.0 20130212082553. Tests pass where expected.

Comment 18

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

Comment on attachment 714259 [details] [diff] [review]
clean up trailing blank lines (beta)

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/76d76cf9fb4b

Comment 19

[Jonathan French (:jfrench)]

Release patch arriving momentarily.

Comment 20

[Jonathan French (:jfrench)]

Attachment: clean up trailing blank lines (release)

Patch "clean up trailing blank lines (release)" for the release branch. Thirty-four files modified. Again, a different set of files in release, hence a separate patch.

Tested with Release 18.0.2 20130201065344. Tests pass where expected.

Comment 21

[Jonathan French (:jfrench)]

Esr17 patch arriving momentarily.

Comment 22

[Jonathan French (:jfrench)]

Attachment: clean up trailing blank lines (esr17)

Patch "clean up trailing blank lines (esr17)" for the esr17 branch. Thirty-three files modified.

Tested with Esr17 17.0.2esrpre 20130215034502. Tests pass where expected.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Release is no longer necessary given that we merged beta to release yesterday.

Comment 24

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

Comment on attachment 714689 [details] [diff] [review]
clean up trailing blank lines (esr17)

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

Looks great, thanks Jonathan!

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

Comment 25

[Jonathan French (:jfrench)]

Cool, so we are done here. As for item 1), I have updated the Mozmill Style Guide whitespace section this morning with a one-liner on trailing blank lines in a file. I've placed in in front of the line describing newline characters, to the order of precedence is hopefully enough to add clarity for new contributors. If anyone wants to have at that other line describing "newline" characters eg. with a code block example or external reference feel free to do so.