Closed Bug 1013518 Opened 5 years ago Closed 4 years ago

Separate DevTools from general browser CSS linting

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jryans, Assigned: Gijs)

References

Details

Attachments

(2 files)

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.
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.
Attachment #8426281 - Flags: review?(bgrinstead)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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
Attachment #8426281 - Flags: review?(bgrinstead) → review+
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. :-)
Keywords: leave-open
Depends on: 1014062
Comment on attachment 8426281 [details] [diff] [review]
include CSS linter test in mochitest-devtools,

remote:   https://hg.mozilla.org/integration/fx-team/rev/7908b4088529
Attachment #8426281 - Flags: checkin+
Depends on: 1062821
Bug 1013518 - separate devtools css linting, r?jryans
Attachment #8660867 - Flags: review?(jryans)
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.
Attachment #8660867 - Flags: review?(jryans) → review+
(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?
Flags: needinfo?(jryans)
(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.
Flags: needinfo?(jryans)
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.