[Tests]: add weekly scheduled smoke tests #2963

AriPerkkio · 2021-04-10T06:27:01Z

Context: AriPerkkio/eslint-remote-tester#29

What

Adds weekly scheduled smoke tests to be run on every Sunday at 00:00. Tests can also be trigger manually (workflow_dispatch). These tests will be looking for rule crashes.

Previously these tests have been run in eslint-remote-tester's repository. So far bugs below have been found by automated CI runs:

jsx-no-constructed-context-values: Cannot read property 'set' of undefined jsx-no-constructed-context-values: Cannot read property 'set' of undefined #2894
jsx-no-constructed-context-values: Cannot read property 'type' of null jsx-no-constructed-context-values: Cannot read property 'type' of null #2895
no-unknown-property: allowedTags.indexOf is not a function no-unknown-property: allowedTags.indexOf is not a function #2879
jsx-max-depth: Maximum call stack size exceeded jsx-max-depth: Maximum call stack size exceeded #2880
jsx-wrap-multilines: RangeError: Invalid count value jsx-wrap-multilines: RangeError: Invalid count value #2875
jsx-no-script-url: Cannot read property 'type' of null jsx-no-script-url: Cannot read property 'type' of null #2871
no-typos: Cannot read property 'toLowerCase' of undefined no-typos: Cannot read property 'toLowerCase' of undefined #2870
jsx-max-depth: Cannot read property 'length' of undefined jsx-max-depth: Cannot read property 'length' of undefined #2869
Unreleased: Add new rule "react/no-referential-type-default-prop" [New] add no-object-type-as-default-prop rule #2848
forbid-dom-props forbid-dom-props: support JSXNamespacedName #2961 (comment)

How

Plugin maintainer making sure all existing rules do not crash

Utilizes https://github.com/marketplace/actions/eslint-remote-tester-runner. Each test run will check as many repositories as it can in 5 hours 30 minutes (default value of timeLimit).
There are 10K repositories in test/smoke-test-repositories.json. The first ~3300 are specifically picked for this plugin as they have /react/ in the repository name. These were collected from libraries.io.

At first the rules will be extends: ['plugin:react/all'] but it is absolutely OK to include rules: {} with settings for specific rules if wanted.

Available configuration options:

eslint-remote-tester-run-action: https://github.com/marketplace/actions/eslint-remote-tester-runner#action-parameters
eslint-remote-tester: https://github.com/AriPerkkio/eslint-remote-tester#configuration-options

When test fails the @github-actions bot will open new issue with title Results of weekly scheduled smoke test. It will reuse the existing issue if previous one hasn't yet been closed. It's recommended to close the issues once their findings are fixed.

🎉

AriPerkkio · 2021-04-10T07:24:17Z

CI's pretest steps are failing due to eslint version conflicts between dependencies.

npm ERR!   peer eslint@"^5.0.0 || ^6.0.0" from @typescript-eslint/[email protected]

npm ERR! Conflicting peer dependency: [email protected]
npm ERR!   peer eslint@">=7" from [email protected]

The reason for eslint-remote-tester's eslint@^7 requirement is the usage of new ESLint class Nodejs API + https://eslint.org/docs/user-guide/migrating-to-7.0.0#deprecate-cliengine.

~~I'm not sure how to resolve this conflict here.~~

Merging #2825 should fix this. Or even @typescript-eslint/parser@^3.10.1 should be enough.

AriPerkkio · 2021-04-11T11:13:27Z

Second and last conflict: version util test was checking actual installed dependencies. Now that react is installed (required by ink) the test fails.
Fixed by [Tests]: version util test to mock "no react installed" case.

ljharb · 2021-04-19T02:39:43Z

package.json

@@ -45,13 +45,14 @@
    "@types/eslint": "^7.2.8",
    "@types/estree": "^0.0.47",
    "@types/node": "^14.14.37",
-    "@typescript-eslint/parser": "^2.34.0",
+    "@typescript-eslint/parser": "^3.10.1",


this seems like it would affect our ability to test on v2 of the parser. any thoughts/concerns?

I've been looking at this changelog of 2.34.0 -> 3.0.0 and don't think there's anything special in v2 that is being missing after upgrading to v3. Most important thing is the node v8 support being dropped out but I think that's it. But I must say I'm not expert when it comes to AST parsers. 🙃

That means we wouldn't be able to use it on node 4 and 6 tests :-/

That's definitely a drawback. Not sure how to resolve this. 😕

A bit off-topic but just to clarify - how was the v2.34.0 be able to run on node 4? It requires node v8 + eslint v5. ESLint v5 requires node 6. Am I reading these requirements correctly?

actually you're right; the tests are set up to skip the TS tests on node < 5 (or maybe <= 5).

(this project supports down to eslint 3, which requires node 4)

Alright so previously with v2.34.0 the TS tests have been run for eslint@^5 + node@^8.
Now the v3 supports eslint@^5 and node@^10. The only thing that is lost is being able to test the typescript parser against node v8. ESLint version is still the same. That doesn't sound too bad, right?

I think we can fix this by depending on ^2.34.0 || ^3.10.1, and in the CI config, manually installing v2 when it's node < 10.

Now using "@typescript-eslint/parser": "^2.34.0 || ^3.10.1" and CI seems to be passing. Does it still require changes to CI workflow configurations?

Should the version be forced here?

https://github.com/yannickcr/eslint-plugin-react/blob/f233eb77c5c55662287991a914f3a7e2c4172db0/.github/workflows/node-4%2B.yml#L65

Yes, I think that's precisely the solution that's needed :-)

CI will now resolve version based on matrix.node: "@typescript-eslint/parser@${{ matrix.node-version >= 10 && '3' || '2' }}".
The "ternary" syntax here is a bit weird but it works.

Logs from jobs:

latest majors (10, 7): * AFTER_INSTALL: npm install --no-save "eslint@7" "@typescript-eslint/parser@3"

latest majors (9, 6): * AFTER_INSTALL: npm install --no-save "eslint@6" "@typescript-eslint/parser@2"

ljharb · 2021-04-19T15:53:17Z

hm, my other comment got lost. can the "list of repos" be fetched remotely? i don't really want to have to maintain a manual list of other people's repos if i can avoid it. (it's great to be able to override it locally if needed, ofc)

AriPerkkio · 2021-04-19T16:25:47Z

That's something other developers have been asking too. These repositories have been fetched with a script for libraries.io but their service timeouts often and is extremely unstable, librariesio/libraries.io#2647. Maybe I should finally build some kind of centralized solution for this.

Plugin projects could fetch these repositories from a hosted gist.github but that means they would need node-fetch or similar request library as dependency.
Maybe a better solution would be if I maintained a separate npm package which provided these repositories? Updating them would be easy, and overriding a specific repository in plugin project would be possible, e.g.

// eslint-remote-tester.config.js
const repositories = require('eslint-remote-tester-repositories');

module.exports = {
   repositories: repositories.filter(repository => repository !== 'org/some-repository-which-we-dont-want'),
   ...
}

I would like to avoid providing these from eslint-remote-tester in order to avoid publishing it often, so a separate npm package would be better.

ljharb · 2021-04-19T17:07:51Z

In the github action, it could be fetched with curl and then cached using the actions cache? but yes a separate npm package would also be fine.

codecov-commenter · 2021-04-21T15:12:43Z

Codecov Report

Merging #2963 (0894132) into master (be06fa4) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2963   +/-   ##
=======================================
  Coverage   97.38%   97.38%           
=======================================
  Files         111      111           
  Lines        7426     7426           
  Branches     2713     2713           
=======================================
  Hits         7232     7232           
  Misses        194      194

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be06fa4...0894132. Read the comment docs.

AriPerkkio · 2021-04-21T15:21:40Z

Now using repositories from eslint-remote-tester-repositories package.

Also a notable change - removed passing ${{ secrets.GITHUB_TOKEN }}. It is no longer required by eslint-remote-tester-run-action. It's using github.token internally which is available by default.

.github/workflows/smoke-test.yml

ljharb · 2021-04-28T16:05:56Z

.github/workflows/smoke-test.yml

+    - run: |
+        npm install
+        npm link
+        npm link eslint-plugin-react


i'm a bit confused why this is needed - npm link alone ensures that the binary is globally available

ESLint is unable load the plugin without this. As far as I understand, the npm link <package> creates the actual symlink under projects node_modules. Does this mean ESLint is unable to use globally available packages?

Here is example run without this line. https://github.com/AriPerkkio/eslint-plugin-react/runs/2477044926?check_suite_focus=true

Error: Configuration validation errors: - eslintrc: Failed to load plugin 'react' declared in 'CLIOptions': Cannot find module 'eslint-plugin-react'

ahhh yes, because eslint requires it.

in that case, an alternative to "link to global, then link back down" is if we add to devDeps "eslint-plugin-react": "link:./", perhaps?

"link:./" seems to fail but "file:./" works. No need to run link steps in workflow now. https://docs.npmjs.com/cli/v7/configuring-npm/package-json#local-paths

"eslint-plugin-react": "file:./",

Oh actually I'll need to revert this change. Some older node versions seem to fail. Some fail in unit-test step, some fail in installation step.

https://github.com/yannickcr/eslint-plugin-react/actions/runs/803786800

seems it behaves differently in npm 6 vs 7 :-/

.github/workflows/smoke-test.yml

.gitignore

ljharb · 2021-04-28T16:08:25Z

package.json

@@ -45,13 +45,14 @@
    "@types/eslint": "^7.2.8",
    "@types/estree": "^0.0.47",
    "@types/node": "^14.14.37",
-    "@typescript-eslint/parser": "^2.34.0",
+    "@typescript-eslint/parser": "^3.10.1",


That means we wouldn't be able to use it on node 4 and 6 tests :-/

AriPerkkio · 2021-04-30T17:08:54Z

Now using ljharb/actions/node/install@main and limiting workflow to yannickcr/eslint-plugin-react repository.

AriPerkkio · 2021-08-09T15:43:39Z

FYI, while testing this setup with latest master it seems that some unreleased changes are causing a lot of crashes for no-typos rule. Also no-unstable-nested-components crashed at least once. Could be related to #3001.

Test has been running for 35mins and found hundreds of crashes: [INFO/STATUS] Errors (617).

- in CI, use the proper version for the node version

ljharb

Awesome, thanks!

AriPerkkio mentioned this pull request Apr 16, 2021

Running eslint-remote-tester-runner for each new version published testing-library/eslint-plugin-testing-library#341

Closed

ljharb reviewed Apr 19, 2021

View reviewed changes

AriPerkkio mentioned this pull request Apr 19, 2021

Publish repositories.json as separate npm package: eslint-remote-tester-repositories AriPerkkio/eslint-remote-tester#71

Closed

[Tests] version util test to mock "no react installed" case

91cb1b4

AriPerkkio force-pushed the test/smoke-tests branch from 4632cea to 547a1ff Compare April 21, 2021 15:11

AriPerkkio force-pushed the test/smoke-tests branch from 547a1ff to 409c10c Compare April 21, 2021 15:13

ljharb reviewed Apr 28, 2021

View reviewed changes

AriPerkkio force-pushed the test/smoke-tests branch 2 times, most recently from b82263e to 338fc3b Compare April 30, 2021 16:51

AriPerkkio force-pushed the test/smoke-tests branch 2 times, most recently from dc6f262 to 238cf8a Compare May 2, 2021 07:22

AriPerkkio force-pushed the test/smoke-tests branch 2 times, most recently from 2ef5e00 to f380d1d Compare May 15, 2021 07:18

ljharb marked this pull request as draft August 9, 2021 06:28

AriPerkkio force-pushed the test/smoke-tests branch from f380d1d to e68f5a1 Compare August 9, 2021 15:03

AriPerkkio mentioned this pull request Aug 9, 2021

no-typos: TypeError: only function nodes are expected #3036

Closed

AriPerkkio added 2 commits August 11, 2021 12:57

[Dev Deps] Update @typescript-eslint/parser

239b661

- in CI, use the proper version for the node version

[Tests] add weekly scheduled smoke tests

0894132

AriPerkkio force-pushed the test/smoke-tests branch from e68f5a1 to 330404f Compare August 11, 2021 12:58

AriPerkkio marked this pull request as ready for review August 11, 2021 13:12

ljharb force-pushed the test/smoke-tests branch from 330404f to 0894132 Compare August 12, 2021 05:30

ljharb approved these changes Aug 12, 2021

View reviewed changes

ljharb merged commit 0894132 into jsx-eslint:master Aug 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tests]: add weekly scheduled smoke tests #2963

[Tests]: add weekly scheduled smoke tests #2963

AriPerkkio commented Apr 10, 2021

AriPerkkio commented Apr 10, 2021 •

edited

Loading

AriPerkkio commented Apr 11, 2021

ljharb Apr 19, 2021

AriPerkkio Apr 19, 2021

ljharb Apr 28, 2021

AriPerkkio Apr 30, 2021

ljharb Apr 30, 2021

AriPerkkio May 2, 2021

ljharb Aug 9, 2021

AriPerkkio Aug 9, 2021

ljharb Aug 11, 2021

AriPerkkio Aug 11, 2021

ljharb commented Apr 19, 2021

AriPerkkio commented Apr 19, 2021

ljharb commented Apr 19, 2021

codecov-commenter commented Apr 21, 2021 •

edited

Loading

AriPerkkio commented Apr 21, 2021

ljharb Apr 28, 2021

AriPerkkio Apr 30, 2021

ljharb Apr 30, 2021

AriPerkkio May 2, 2021

AriPerkkio May 2, 2021

ljharb May 2, 2021

ljharb Apr 28, 2021

AriPerkkio commented Apr 30, 2021

AriPerkkio commented Aug 9, 2021

ljharb left a comment

[Tests]: add weekly scheduled smoke tests #2963

[Tests]: add weekly scheduled smoke tests #2963

Conversation

AriPerkkio commented Apr 10, 2021

What

How

AriPerkkio commented Apr 10, 2021 • edited Loading

AriPerkkio commented Apr 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Apr 19, 2021

AriPerkkio commented Apr 19, 2021

ljharb commented Apr 19, 2021

codecov-commenter commented Apr 21, 2021 • edited Loading

Codecov Report

AriPerkkio commented Apr 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AriPerkkio commented Apr 30, 2021

AriPerkkio commented Aug 9, 2021

ljharb left a comment

Choose a reason for hiding this comment

AriPerkkio commented Apr 10, 2021 •

edited

Loading

codecov-commenter commented Apr 21, 2021 •

edited

Loading