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

Use ES6 on visual tests #36915

Merged
merged 3 commits into from Sep 21, 2022
Merged

Use ES6 on visual tests #36915

merged 3 commits into from Sep 21, 2022

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Aug 6, 2022

Merge after #36914

@GeoSot GeoSot added the tests label Aug 6, 2022
@GeoSot GeoSot force-pushed the gs/use-es6-on-visual-tests branch from c1b6feb to 886f961 Compare August 6, 2022 21:50
@GeoSot GeoSot marked this pull request as ready for review September 7, 2022 08:56
@julien-deramond
Copy link
Member

julien-deramond commented Sep 14, 2022

Of course not mandatory for this PR: I just tried quickly but what would you think about adding some automation here with eslint-plugin-html?

Configuration would look like something like this:

diff --git a/.eslintignore b/.eslintignore
index a18b03a5d..04bae1541 100644
--- a/.eslintignore
+++ b/.eslintignore
@@ -3,4 +3,6 @@
 **/vendor/
 /_site/
 /js/coverage/
+/js/tests/integration/
 /site/static/sw.js
+/site/layouts/
diff --git a/js/tests/unit/.eslintrc.json b/js/tests/unit/.eslintrc.json
index 6362a1acf..6508fc062 100644
--- a/js/tests/unit/.eslintrc.json
+++ b/js/tests/unit/.eslintrc.json
@@ -1,4 +1,7 @@
 {
+  "plugins": [
+    "html"
+  ],
   "extends": [
     "../../../.eslintrc.json"
   ],
diff --git a/package.json b/package.json
index 50d4b35db..4db97cff1 100644
--- a/package.json
+++ b/package.json
@@ -63,7 +63,7 @@
     "js-compile-standalone-esm": "rollup --environment ESM:true,BUNDLE:false --config build/rollup.config.js --sourcemap",
     "js-compile-bundle": "rollup --environment BUNDLE:true --config build/rollup.config.js --sourcemap",
     "js-compile-plugins": "node build/build-plugins.js",
-    "js-lint": "eslint --cache --cache-location .cache/.eslintcache --report-unused-disable-directives .",
+    "js-lint": "eslint --cache --cache-location .cache/.eslintcache --report-unused-disable-directives --ext .html,.js .",
     "js-minify": "npm-run-all --aggregate-output --parallel js-minify-*",
     "js-minify-standalone": "terser --compress passes=2 --mangle --comments \"/^!/\" --source-map \"content=dist/js/bootstrap.js.map,includeSources,url=bootstrap.min.js.map\" --output dist/js/bootstrap.min.js dist/js/bootstrap.js",
     "js-minify-standalone-esm": "terser --compress passes=2 --mangle --comments \"/^!/\" --source-map \"content=dist/js/bootstrap.esm.js.map,includeSources,url=bootstrap.esm.min.js.map\" --output dist/js/bootstrap.esm.min.js dist/js/bootstrap.esm.js",
@@ -118,6 +118,7 @@
     "cross-env": "^7.0.3",
     "eslint": "^8.23.1",
     "eslint-config-xo": "^0.42.0",
+    "eslint-plugin-html": "^7.1.0",
     "eslint-plugin-import": "^2.26.0",
     "eslint-plugin-markdown": "^3.0.0",
     "eslint-plugin-unicorn": "^42.0.0",

And a new js/tests/visual/.eslintrc.json:

{
  "plugins": [
    "html"
  ],
  "extends": "../../../.eslintrc.json",
  "parserOptions": {
    "sourceType": "module"
  },
  "settings": {
    "html/html-extensions": [".html"]
  }
}

@GeoSot
Copy link
Member Author

GeoSot commented Sep 14, 2022

Of course not mandatory for this PR: I just tried quickly but what would you think about adding some automation here with eslint-plugin-html?

Linting is always a nice idea. Would you mind addressing it on a different PR?

@GeoSot GeoSot force-pushed the gs/use-es6-on-visual-tests branch 2 times, most recently from af3a8cc to 77b9134 Compare September 15, 2022 11:09
js/tests/visual/modal.html Outdated Show resolved Hide resolved
Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

IMO those modifications are OK before we have something to check everything automatically.
Tested manually the files and 👌

Just for information, here is the report done by eslint-plugin-html without fine tuning it by removing useless rules. Maybe there are some things to you'd like to integrate right now in the modifications 🤷

/home/ju/bootstrap/js/tests/visual/carousel.html
  58:9  error  Unexpected console statement  no-console

/home/ju/bootstrap/js/tests/visual/modal.html
  214:63  error  Use `for…of` instead of `.forEach(…)`                        unicorn/no-array-for-each
  214:88  error  'bootstrap' is not defined                                   no-undef
  216:63  error  Use `for…of` instead of `.forEach(…)`                        unicorn/no-array-for-each
  216:88  error  'bootstrap' is not defined                                   no-undef
  220:9   error  This `if` statement can be replaced by a ternary expression  unicorn/prefer-ternary
  234:7   error  Expected blank line before this statement                    padding-line-between-statements
  236:27  error  'bootstrap' is not defined                                   no-undef
  249:13  error  Unexpected console statement                                 no-console
  251:13  error  Unexpected console statement                                 no-console
  268:9   error  Unexpected console statement                                 no-console

/home/ju/bootstrap/js/tests/visual/popover.html
  36:63  error  Use `for…of` instead of `.forEach(…)`  unicorn/no-array-for-each
  36:88  error  'bootstrap' is not defined             no-undef

/home/ju/bootstrap/js/tests/visual/toast.html
  56:45  error  Use `for…of` instead of `.forEach(…)`     unicorn/no-array-for-each
  56:68  error  'bootstrap' is not defined                no-undef
  59:47  error  Use `for…of` instead of `.forEach(…)`     unicorn/no-array-for-each
  59:65  error  Multiple spaces found before 'bootstrap'  no-multi-spaces
  59:67  error  'bootstrap' is not defined                no-undef
  63:47  error  Use `for…of` instead of `.forEach(…)`     unicorn/no-array-for-each
  63:65  error  Multiple spaces found before 'bootstrap'  no-multi-spaces
  63:67  error  'bootstrap' is not defined                no-undef

/home/ju/bootstrap/js/tests/visual/tooltip.html
   98:9   error  Do not use 'new' for side effects             no-new
   98:13  error  'bootstrap' is not defined                    no-undef
   99:9   error  Do not use 'new' for side effects             no-new
   99:13  error  'bootstrap' is not defined                    no-undef
  104:1   error  More than 1 blank line not allowed            no-multiple-empty-lines
  105:7   error  Do not use 'new' for side effects             no-new
  105:11  error  'bootstrap' is not defined                    no-undef
  109:33  error  'bootstrap' is not defined                    no-undef
  110:18  error  Extra space after key 'placement'             key-spacing
  111:16  error  Extra space after key 'trigger'               key-spacing
  115:63  error  Use `for…of` instead of `.forEach(…)`         unicorn/no-array-for-each
  115:71  error  Missing space before =>                       arrow-spacing
  115:82  error  Multiple spaces found before 'new'            no-multi-spaces
  115:88  error  'bootstrap' is not defined                    no-undef
  119:7   error  Do not use 'new' for side effects             no-new
  119:11  error  'bootstrap' is not defined                    no-undef
  124:16  error  'duplicateButtons' is defined but never used  no-unused-vars
  127:36  error  Operator '+=' must be spaced                  space-infix-ops
  128:7   error  Expected indentation of 2 spaces but found 1  indent

js/tests/visual/modal.html Outdated Show resolved Hide resolved
@GeoSot
Copy link
Member Author

GeoSot commented Sep 21, 2022

Just for information, here is the report done by eslint-plugin-html without fine tuning it by removing useless rules. Maybe there are some things to you'd like to integrate right now in the modifications 🤷

Probably it will need an eslintrc.json override like bootstrap\site\.eslintrc.json

We can fix any issues there 😉

Thank you for your help @julien-deramond

@GeoSot GeoSot merged commit 27f2025 into main Sep 21, 2022
@GeoSot GeoSot deleted the gs/use-es6-on-visual-tests branch September 21, 2022 22:48
@XhmikosR XhmikosR changed the title Use es6 on visual tests Use ES6 on visual tests Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants