1194314 - ESLint cleanup for tests

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect)

Comment 1

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1194314.patch

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6328ce1c2019

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8648115 [details] [diff] [review]
Bug1194314.patch

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

Thanks! r=me with the comments addressed

::: browser/devtools/styleeditor/test/browser_styleeditor_inline_friendly_names.js
@@ +42,5 @@
>      "URI is the identifier of style sheet file.");
>  
> +  is(ui.getStyleSheetIdentifier(fakeInlineStyleSheet),
> +    "inline-2-at-http://example.com/",
> +    "Inline style sheets are identified by their page and position at that " +

Rather than wrapping the line, could we rephrase to something like:

"Inline sheets are identified by their page and position in the page."

@@ +70,4 @@
>    is(firstEditor.friendlyName, SAVE_PATH,
>      "Friendly name is correct for the saved inline style sheet.");
>    isnot(secondEditor.friendlyName, SAVE_PATH,
> +    "Friendly name is for the second inline style sheet is not the same as " +

Rather than wrapping the line, could we rephrase to something like:

"Friendly name for the second inline sheet isn't the same as the first."

::: browser/devtools/styleeditor/test/browser_styleeditor_pretty.js
@@ +7,5 @@
>  // untouched.
>  
>  const TESTCASE_URI = TEST_BASE_HTTP + "minified.html";
>  
> +/*

Should this be two different comments?  One above the declaration for PRETTIFIED_SOURCE and one above ORIGINAL_SOURCE

::: browser/devtools/styleeditor/test/browser_styleeditor_reload.js
@@ +20,5 @@
>    yield ui.selectStyleSheet(ui.editors[1].styleSheet, LINE_NO, COL_NO);
>  
>    info("Reloading page.");
> +  executeInContent("devtools:test:reload", {}, {},
> +    /* no response */

Should false be on the same line as the comment here?

::: browser/devtools/styleeditor/test/browser_styleeditor_sourcemap_watching.js
@@ +137,5 @@
>      .getService(Ci.nsIScriptableInputStream);
>  
> +  let channel = Services.io.newChannel2(srcChromeURL,
> +                                        null,
> +                                        null,

Is there a rule about not having this comment on the same line as the arg, or is it that this is > 80 characters?  b/c I think the comment right after 'null' makes it more clear

@@ +143,2 @@
>                                          null,
> +                                        Services.scriptSecurityManager

if it would help, you could have a line above like 

`let principal = Services.scriptSecurityManager.getSystemPrincipal();`

then use principal in the argument list.

::: browser/devtools/styleeditor/test/browser_styleeditor_sourcemaps.js
@@ +43,5 @@
>      "",
> +    "/*# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJma" +
> +    "WxlIjoiIiwic291cmNlcyI6WyJzYXNzL2NvbnRhaW5lZC5zY3NzIl0sIm5hbWVzIjpbXSwi" +
> +    "bWFwcGluZ3MiOiJBQUVBO0VBQ0UsT0FISyIsInNvdXJjZXNDb250ZW50IjpbIiRwaW5rOiA" +
> +    "jZjA2O1xuXG4jaGVhZGVyIHtcbiAgY29sb3I6ICRwaW5rO1xufSJdfQ==*/"

I think we should have an exception to the rules for data URIs or other necessarily long strings.  What do you think?  I'm not crazy about this change, but it's fine.

Comment 3

[Jen Fong-Adwent [:ednapiranha]]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Comment on attachment 8648115 [details] [diff] [review]
> Bug1194314.patch
> 
> Review of attachment 8648115 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks! r=me with the comments addressed
> 
> :::
> browser/devtools/styleeditor/test/browser_styleeditor_inline_friendly_names.
> js
> @@ +42,5 @@
> >      "URI is the identifier of style sheet file.");
> >  
> > +  is(ui.getStyleSheetIdentifier(fakeInlineStyleSheet),
> > +    "inline-2-at-http://example.com/",
> > +    "Inline style sheets are identified by their page and position at that " +
> 
> Rather than wrapping the line, could we rephrase to something like:
> 
> "Inline sheets are identified by their page and position in the page."

Yep, I'll reword.

> 
> @@ +70,4 @@
> >    is(firstEditor.friendlyName, SAVE_PATH,
> >      "Friendly name is correct for the saved inline style sheet.");
> >    isnot(secondEditor.friendlyName, SAVE_PATH,
> > +    "Friendly name is for the second inline style sheet is not the same as " +
> 
> Rather than wrapping the line, could we rephrase to something like:
> 
> "Friendly name for the second inline sheet isn't the same as the first."
> 

Yep, I'll reword.

> ::: browser/devtools/styleeditor/test/browser_styleeditor_pretty.js
> @@ +7,5 @@
> >  // untouched.
> >  
> >  const TESTCASE_URI = TEST_BASE_HTTP + "minified.html";
> >  
> > +/*
> 
> Should this be two different comments?  One above the declaration for
> PRETTIFIED_SOURCE and one above ORIGINAL_SOURCE

Yes, you are correct.

> 
> ::: browser/devtools/styleeditor/test/browser_styleeditor_reload.js
> @@ +20,5 @@
> >    yield ui.selectStyleSheet(ui.editors[1].styleSheet, LINE_NO, COL_NO);
> >  
> >    info("Reloading page.");
> > +  executeInContent("devtools:test:reload", {}, {},
> > +    /* no response */
> 
> Should false be on the same line as the comment here?
> 
> :::
> browser/devtools/styleeditor/test/browser_styleeditor_sourcemap_watching.js
> @@ +137,5 @@
> >      .getService(Ci.nsIScriptableInputStream);
> >  
> > +  let channel = Services.io.newChannel2(srcChromeURL,
> > +                                        null,
> > +                                        null,
> 
> Is there a rule about not having this comment on the same line as the arg,
> or is it that this is > 80 characters?  b/c I think the comment right after
> 'null' makes it more clear
> 

Yep, there is an ESLint rule about not having comments on the same line as code - as discussed on IRC, I'll just remove the comment.

> @@ +143,2 @@
> >                                          null,
> > +                                        Services.scriptSecurityManager
> 
> if it would help, you could have a line above like 
> 
> `let principal = Services.scriptSecurityManager.getSystemPrincipal();`
> 
> then use principal in the argument list.
> 
> ::: browser/devtools/styleeditor/test/browser_styleeditor_sourcemaps.js
> @@ +43,5 @@
> >      "",
> > +    "/*# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJma" +
> > +    "WxlIjoiIiwic291cmNlcyI6WyJzYXNzL2NvbnRhaW5lZC5zY3NzIl0sIm5hbWVzIjpbXSwi" +
> > +    "bWFwcGluZ3MiOiJBQUVBO0VBQ0UsT0FISyIsInNvdXJjZXNDb250ZW50IjpbIiRwaW5rOiA" +
> > +    "jZjA2O1xuXG4jaGVhZGVyIHtcbiAgY29sb3I6ICRwaW5rO1xufSJdfQ==*/"
> 
> I think we should have an exception to the rules for data URIs or other
> necessarily long strings.  What do you think?  I'm not crazy about this
> change, but it's fine.

Yeah I thought about it, but I decided to be consistent w/ the rules - I'm okay either way but it does create a warning about lines exceeding some point n (in this case, 80). I could revert but it just means it makes it more confusing for contributors if/when they have to either a) follow the ESLint rules exactly or b) have exceptions which we internally allow/deny. If we do agree with b) then we should outline this somewhere in our docs or we should adjust the .eslintrc file to not complain about it. Thoughts?

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Jen Fong-Adwent [:ednapiranha] from comment #3)
> > ::: browser/devtools/styleeditor/test/browser_styleeditor_sourcemaps.js
> > @@ +43,5 @@
> > >      "",
> > > +    "/*# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJma" +
> > > +    "WxlIjoiIiwic291cmNlcyI6WyJzYXNzL2NvbnRhaW5lZC5zY3NzIl0sIm5hbWVzIjpbXSwi" +
> > > +    "bWFwcGluZ3MiOiJBQUVBO0VBQ0UsT0FISyIsInNvdXJjZXNDb250ZW50IjpbIiRwaW5rOiA" +
> > > +    "jZjA2O1xuXG4jaGVhZGVyIHtcbiAgY29sb3I6ICRwaW5rO1xufSJdfQ==*/"
> > 
> > I think we should have an exception to the rules for data URIs or other
> > necessarily long strings.  What do you think?  I'm not crazy about this
> > change, but it's fine.
> 
> Yeah I thought about it, but I decided to be consistent w/ the rules - I'm
> okay either way but it does create a warning about lines exceeding some
> point n (in this case, 80). I could revert but it just means it makes it
> more confusing for contributors if/when they have to either a) follow the
> ESLint rules exactly or b) have exceptions which we internally allow/deny.
> If we do agree with b) then we should outline this somewhere in our docs or
> we should adjust the .eslintrc file to not complain about it. Thoughts?

Let's forward this to Patrick.  My suggestion is we add a new rule that looks for something like \"(.*)data:(.+)/(.+);(.+)\" in the line and then allows it to exceed 80 characters.  Note: I don't know how possible this would be, but it'd be good if we get his opinion about whether that would even be desirable.

In the meantime, let's land it how you have it (so it's passing the linter as-is).

Comment 5

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1194314.patch

Comment 6

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to Jen Fong-Adwent [:ednapiranha] from comment #3)
> > > ::: browser/devtools/styleeditor/test/browser_styleeditor_sourcemaps.js
> > > @@ +43,5 @@
> > > >      "",
> > > > +    "/*# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJma" +
> > > > +    "WxlIjoiIiwic291cmNlcyI6WyJzYXNzL2NvbnRhaW5lZC5zY3NzIl0sIm5hbWVzIjpbXSwi" +
> > > > +    "bWFwcGluZ3MiOiJBQUVBO0VBQ0UsT0FISyIsInNvdXJjZXNDb250ZW50IjpbIiRwaW5rOiA" +
> > > > +    "jZjA2O1xuXG4jaGVhZGVyIHtcbiAgY29sb3I6ICRwaW5rO1xufSJdfQ==*/"
> > > 
> > > I think we should have an exception to the rules for data URIs or other
> > > necessarily long strings.  What do you think?  I'm not crazy about this
> > > change, but it's fine.
> > 
> > Yeah I thought about it, but I decided to be consistent w/ the rules - I'm
> > okay either way but it does create a warning about lines exceeding some
> > point n (in this case, 80). I could revert but it just means it makes it
> > more confusing for contributors if/when they have to either a) follow the
> > ESLint rules exactly or b) have exceptions which we internally allow/deny.
> > If we do agree with b) then we should outline this somewhere in our docs or
> > we should adjust the .eslintrc file to not complain about it. Thoughts?
> 
> Let's forward this to Patrick.  My suggestion is we add a new rule that
> looks for something like \"(.*)data:(.+)/(.+);(.+)\" in the line and then
> allows it to exceed 80 characters.  Note: I don't know how possible this
> would be, but it'd be good if we get his opinion about whether that would
> even be desirable.
Since we added the .eslintrc files, we started noticing things like this. Rules that work for us 99% of the times, but there's this 1% cases where we'd like the rule to just not be there. As you said, either we live with this and write something in the doc (that no one will ever read), or we make another rule.
ESLint was chosen also because it's very easily extensible, so I'd say, let's use this capability and start creating our own rules.
However, it turns out the max-len rule already support a parameter called ignorePattern which we could use to skip over data-uris: http://eslint.org/docs/rules/max-len#options

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9e51cc2ce944

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/9e51cc2ce944

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9e51cc2ce944