1285936 - Update eslint plugin after browser-fullScreen.js was renamed to browser-fullScreenAndPointerLock.js

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect)

Description

[Julian Descottes [:jdescottes]]

The mozilla eslint plugin reports an error in all files after Bug 1273351.

"Could not load globals from file browser/base/content/browser-fullScreen.js"

The plugin maintains a list of files from which globals are imported in /tools/lint/eslint/eslint-plugin-mozilla/lib/rules/import-browserjs-globals.js

Since browser-fullScreen.js was renamed to browser-fullScreenAndPointerLock, the references to this file should be updated.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1285936 - eslint mozilla plugin: fix filename in import globals & bump to 0.1.1

The file base/content/browser-fullScreen.js was renamed. This patch updates the reference
to this filename in the esling rule import-browserjs-globals.js and bumps the version to
0.1.1.

Review commit: https://reviewboard.mozilla.org/r/63452/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63452/

Comment 2

[Julian Descottes [:jdescottes]]

Comment on attachment 8769663 [details]
Bug 1285936 - eslint mozilla plugin: fix filename in import globals & bump to 0.1.1

Sorry for the flag, not intended for review yet.

Comment 3

[Julian Descottes [:jdescottes]]

jryans: I would like to update the mozilla eslint plugin but I don't know how to update tools/lint/eslint/manifest.tt . Can you help me here?

Comment 4

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

Attachment: ESLint update changes to fold

I have updated the ESLint files, here are the additional changes you should fold into your patch.

With this added, you can do a try run and see your changes applied there.

Comment 5

[Julian Descottes [:jdescottes]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> Created attachment 8769711 [details]
> ESLint update changes to fold
> 
> I have updated the ESLint files, here are the additional changes you should
> fold into your patch.
> 
> With this added, you can do a try run and see your changes applied there.

Thanks for the patch! I folded this into my patch and sent it to try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5709c47ccd7

We now have failures such as : 

  TEST-UNEXPECTED-ERROR | browser/base/content/socialchat.xml:98:23 | Parsing error: Unexpected token ; (null)

It looks like any XML file containing javascript not wrapped in CDATA is now making eslint fail. I'm guessing this comes from one of the dependencies that got automatically updated when running the update script. I need to figure out which one is responsible.

Here is the list of packages that got updated :

"array-union": "1.0.1" to "1.0.2"
"array-uniq": "1.0.2" to "1.0.3"
"bluebird": "3.4.0" to "3.4.1"
"brace-expansion": "1.1.4" to "1.1.5"
"del": "2.2.0" to "2.2.1"
"es5-ext": "0.10.11" to "0.10.12"
"fs.realpath": (new) "1.0.0"
"glob": "7.0.3" to "7.0.5"
"globals": "9.8.0" to "9.9.0"
"globby": "4.1.0" to "5.0.0"
"htmlparser2": "3.9.0" to "3.9.1"
"ignore": "3.1.2" to "3.1.3"
"minimatch": "3.0.0" to "3.0.2"
"rimraf": "2.5.2" to "2.5.3"

Comment 6

[Patrick Brosset <:pbro>]

Looks like this is getting more complex than originally anticipated.
This started with with a simple update to import-browserjs-globals.js, but now caused a weird XML parsing error. Not sure Julian is the best placed to fix this bug anymore, also because making version changes requires special rights that he does not have.

The problem here is, once again, that new warnings have been introduced without people realizing.
We should not have warnings, only errors, because they make the job fail, and so people notice. Warnings are completely ignored but do cause confusion later for other people when reading the logs. So we should avoid them at all costs (or fail the jobs for warnings too).
So really, this is a regression from bug 1273351 that should have been spotted on CI, but wasn't.

Needinfo for Dave who added import-browserjs-globals.js in the first place: do you mind taking a look at this? Or know someone who could (since you might be busy on Tofino?)

Comment 7

[Julian Descottes [:jdescottes]]

For the record, the warning level here is coming from 

  "rules": {
    "mozilla/import-headjs-globals": 1,
    "mozilla/import-browserjs-globals": 1,
  }

defined in testing/mochitest/browser.eslintrc

Comment 8

[Kris Maglione [:kmag]]

While we're at it, can we also change this so that missing files trigger an error rather than a warning, so that breakage like this doesn't land again?

Comment 9

[Julian Descottes [:jdescottes]]

Unassigning myself, can't efficiently investigate here. This needs to be picked up by someone who has the appropriate rights to update the plugin.

Comment 10

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

It looks like the updated modules are fine.  Instead, the parsing issue is from other changes to the Mozilla plugin that were never packaged for automation (bug 1280883).  I will back out the other bug so we can make progress here.

Comment 11

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20d9c46a1aa4

Comment 12

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

(In reply to Kris Maglione [:kmag] from comment #8)
> While we're at it, can we also change this so that missing files trigger an
> error rather than a warning, so that breakage like this doesn't land again?

I went ahead and changed this to an error for now as well to hopefully avoid this particular issue in the future.  Sounds like we may also change warnings to fail in automation as future work, but this change seemed prudent for the moment.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9131153c7c48
eslint mozilla plugin: fix filename in import globals & bump to 0.1.1
https://hg.mozilla.org/integration/mozilla-inbound/rev/1915421654bd
Make import-*-globals rules into errors for automation. r=me

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9131153c7c48
https://hg.mozilla.org/mozilla-central/rev/1915421654bd