ni / javascript-styleguide

JavaScript and TypeScript Style Guide
MIT License
9 stars 9 forks source link

NI ESLint ruleset for Playwright #114

Closed jattasNI closed 1 year ago

jattasNI commented 1 year ago

Several codebases are using Playwright for end-to-end testing. They often depend on eslint-plugin-playwright and configure their eslintrc like this:

           "extends": [
                "@ni/eslint-config-typescript",
                "@ni/eslint-config-typescript/requiring-type-checking",
                "plugin:playwright/playwright-test"
            ],

One problem with this configuration is that playwright/playwright-test has several rules configured as warning but we generally prefer to use error or off because it prevents lint output from getting noisy with warnings that devs don't bother to fix.

If we publish a Playwright-specific configuration we should also:

  1. have it extend @ni/eslint-config-typescript so clients don't need to extend as many rulesets. Having it in a separate config from our typescript ruleset would allow it to be targeted to specific files with the -test.ts extension. We'd also provide a eslint-config-playwright-requiring-type-checking that would be an alias for the existing typescript-requring-type-checking (or decide to get rid of all of our *-requring-type-checking rulesets, but that could be done in a separate pass).
  2. evaluate rules that aren't part of their recommended ruleset to see if we want to enable them by default (see comments below for our decisions)
  3. have a smoke test as we do for other config files
  4. update our scripts that evaluate whether the rules have changed
  5. update docs
  6. evangelize the ruleset to playwright codebases (and/or update those codebases ourselves)
mure commented 1 year ago

It came up in this PR that we may want to disable playwright/no-force-option.

rajsite commented 1 year ago

In meeting went through the default rule set for playwright/playwright-test and came to the following proposals:

no-page-pause: error, seems like a debugging thing and should not be in test no-element-handle: error, prefer using the locator api no-eval: change to error, prefer using the locator api and locator evaluate no-skipped-test: off, having skipped tests is useful, particularly for our e2e tests where workflows require temporarily disabling some tests no-wait-for-timeout: error, do not do arbitrary waits in tests no-force-option: error, making it an error to encourage devs to leave a comment describing why force is required max-nested-describe: error, seems unlikely to hit this case so keeping as an upper bound no-condition-in-test: error, rule description was compelling no-useless-not: error, minor style improvement that makes assertions read more naturally

Edit update 1/26/2023: Of the non-default rules we chose to enable: prefer-to-have-length - error, provides clearer errors require-top-level-describe - error, aligns with patterns we use in jasmine etc so okay to enforce

jattasNI commented 1 year ago

Note for next meeting: in the above list, I believe there were still a few more rules left to evaluate (all the non-default ones).

There has also been a new release of the ruleset (0.12.0) which adds some new rules for us to consider.