Skip to content
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

Merged
merged 2 commits into from Apr 9, 2024
Merged

Conversation

timmywil
Copy link
Member

@timmywil timmywil commented Apr 6, 2024

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.

Copy link
Member

@mgol mgol left a 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

@@ -12,7 +12,8 @@
"globals": {
"fetch": false,
"Promise": false,
"require": false
"require": false,
"Set": false
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@timmywil timmywil merged commit 4af5cae into jquery:main Apr 9, 2024
22 checks passed
@timmywil timmywil deleted the test-align branch April 9, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants