-
Notifications
You must be signed in to change notification settings - Fork 5.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tests: align test runner with other repos #2234
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment, LGTM otherwise
tests/runner/.eslintrc.json
Outdated
@@ -12,7 +12,8 @@ | |||
"globals": { | |||
"fetch": false, | |||
"Promise": false, | |||
"require": false | |||
"require": false, | |||
"Set": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn’t be required. Since we’re running a relatively new Node.js in the runner, we should just declare all globals for a proper ECMAScript version instead of adding them one by one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think so either, but the ecma version was set and they were still throwing errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because setting ecmaVersion does not set the environment - but setting the environment does set the ecmaVersion. You need to add the environment. You can the optionally drop ecmaVersion but I guess it may be easier to migrate to flat config later if it stays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the environment is also set using node: true
above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not this environment; the ECMAScript one. es2022
or whichever version we can rely on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok, I'm not used to using more then node
or browser
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ecmaVersion was already set to 2022
, but that still didn't add globals. However, using es6: true
in the env does work (there is no env for es2022). I like the flat way of doing this better, but this should work for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting; there should be env’s for all ES versions: https://eslint.org/docs/latest/use/configure/language-options-deprecated
I expect to migrate to the flat config eventually, though; it shouldn’t require too much work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This aligns the test runner code with the most recent changes from core, migrate, and color. Workflows have not changed, but browserstack can be run locally.