1013518 - Separate DevTools from general browser CSS linting

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Bug 971096 added linting of all browser CSS.  

Now that DevTools browser chrome tests run in their own dt suite, the DevTools team would prefer to not run the bc suite (to be better citizens with try usage).  However, several engineers have already been bitten by this, since they miss out on linter errors that only show up when running bc.

In order to do this safely, we need to have the current linting test ignore DevTools styles, and create a separate test under the DevTools suite that lints only those files.

Comment 1

[:Gijs (he/him)]

Attachment: include CSS linter test in mochitest-devtools,

Note that filtering mochitest-dt locally will always remove this test (because it's not in a devtools dir) but at least like this it'll run for complete suite runs (./mach mochitest-devtools) and on try.

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8426281 [details] [diff] [review]
include CSS linter test in mochitest-devtools,

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

Sounds good to me.  I pushed to try with dt only and a CSS lint error to confirm that it fails: https://tbpl.mozilla.org/?tree=Try&rev=747b076010e0

Comment 3

[:Gijs (he/him)]

Ideally, we should expose suite info to mochitests, and then we can make the test only fail in one suite depending on where the failing CSS comes from... which will annoy the sheriffs less. :-)

Comment 4

[:Gijs (he/him)]

Comment on attachment 8426281 [details] [diff] [review]
include CSS linter test in mochitest-devtools,

remote:   https://hg.mozilla.org/integration/fx-team/rev/7908b4088529

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7908b4088529

Comment 6

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1013518 - separate devtools css linting, r?jryans

Bug 1013518 - separate devtools css linting, r?jryans

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8660867 [details]
MozReview Request: Bug 1013518 - separate devtools css linting, r?jryans

https://reviewboard.mozilla.org/r/19185/#review17075

Makes sense to me overall!

::: browser/base/content/test/general/browser_parsable_css.js:14
(Diff revision 1)
>    {sourceName: /cleopatra.*(tree|ui)\.css/i},

I believe this no longer exists and can be removed.

::: browser/base/content/test/general/browser_parsable_css.js:16
(Diff revision 1)
>    {sourceName: /codemirror\.css/i},

This should be removable with the general exception you are adding here.

::: browser/base/content/test/general/browser_parsable_css.js:35
(Diff revision 1)
>    {sourceName: /highlighter\.css/i,

This should be removable with the general exception you are adding here.

Comment 8

[:Gijs (he/him)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> ::: browser/base/content/test/general/browser_parsable_css.js:16
> (Diff revision 1)
> >    {sourceName: /codemirror\.css/i},
> 
> This should be removable with the general exception you are adding here.
> 
> ::: browser/base/content/test/general/browser_parsable_css.js:35
> (Diff revision 1)
> >    {sourceName: /highlighter\.css/i,
> 
> This should be removable with the general exception you are adding here.

I'm confused. I'm not adding an exception - the test gets run twice in our code, once as part of the devtools suite, once as part of the normal browser suite. In the former case, the highlighter and codemirror are still going to be tested, and found wanting, right? Am I missing something?

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> > ::: browser/base/content/test/general/browser_parsable_css.js:16
> > (Diff revision 1)
> > >    {sourceName: /codemirror\.css/i},
> > 
> > This should be removable with the general exception you are adding here.
> > 
> > ::: browser/base/content/test/general/browser_parsable_css.js:35
> > (Diff revision 1)
> > >    {sourceName: /highlighter\.css/i,
> > 
> > This should be removable with the general exception you are adding here.
> 
> I'm confused. I'm not adding an exception - the test gets run twice in our
> code, once as part of the devtools suite, once as part of the normal browser
> suite. In the former case, the highlighter and codemirror are still going to
> be tested, and found wanting, right? Am I missing something?

Ah right, my mistake.  So then only the cleopatra one needs removing.

Comment 10

[Pulsebot]

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

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/c4fc1b25416c