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

ci: properly configure smoke test #1003

Merged
merged 5 commits into from Jan 1, 2022
Merged

ci: properly configure smoke test #1003

merged 5 commits into from Jan 1, 2022

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Dec 26, 2021

whoops

@G-Rath G-Rath changed the title ci: adjust ci: properly configure smoke test Dec 26, 2021
@G-Rath G-Rath force-pushed the config-smoke-test branch 2 times, most recently from ff1dc6c to d1fd7eb Compare December 26, 2021 21:55
@G-Rath G-Rath marked this pull request as ready for review December 27, 2021 21:01
@G-Rath G-Rath force-pushed the config-smoke-test branch 3 times, most recently from 36d3fd9 to 35b873d Compare December 31, 2021 23:01
@@ -21,4 +21,3 @@ jobs:
- uses: AriPerkkio/eslint-remote-tester-run-action@v3
with:
issue-title: 'Results of weekly scheduled smoke test'
eslint-remote-tester-config: smoke-test/eslint-remote-tester.config.js

Choose a reason for hiding this comment

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

The action doesn't actually pick these .ts configurations by default since action parameter has default value for .js config: https://github.com/AriPerkkio/eslint-remote-tester-run-action/blob/e72eaaa36788a76ef9044271bebbbd83377e7d6d/action.yml#L14-L17

So this needs to be specified here:

    eslint-remote-tester-config: smoke-test/eslint-remote-tester.config.ts

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yeah I wondered if that might be the case 👍

@@ -111,6 +111,8 @@
"eslint-plugin-import": "^2.25.1",
"eslint-plugin-node": "^11.0.0",
"eslint-plugin-prettier": "^3.4.1",
"eslint-remote-tester": "^2.1.0",

Choose a reason for hiding this comment

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

Suggested change
"eslint-remote-tester": "^2.1.0",
"eslint-remote-tester": "^2.1.1",

@G-Rath
Copy link
Collaborator Author

G-Rath commented Jan 1, 2022

@AriPerkkio thanks for the review - tbh I'm sort of on the fence about sticking with this now that I know you've actually been running the same workflow for our plugin and from what I can tell we've never actually had an error; it seems like a bit of a waste of power to be running the same ~6 hour workflow twice...

@G-Rath
Copy link
Collaborator Author

G-Rath commented Jan 1, 2022

(I'm going to merge now just so it's fixed, but still thinking about maybe reverting)

@G-Rath G-Rath merged commit 88c43dd into main Jan 1, 2022
@G-Rath G-Rath deleted the config-smoke-test branch January 1, 2022 20:33
@G-Rath
Copy link
Collaborator Author

G-Rath commented Jan 1, 2022

@AriPerkkio random other question: does the remote tester support anyway to handle linting repositories multiple times with different rule configurations? My thinking is about how rule logic can be changed with options, so running against e.g. jest/all wouldn't cover all of the possible code the rules could run.

I can't find anything in the docs that look like a way to do this - if I've not missed anything, let me know if you think this could be useful and I can open a new issue to continue discussing.

@github-actions
Copy link

github-actions bot commented Jan 1, 2022

🎉 This PR is included in version 25.3.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@@ -21,4 +21,4 @@ jobs:
- uses: AriPerkkio/eslint-remote-tester-run-action@v3
with:
issue-title: 'Results of weekly scheduled smoke test'
eslint-remote-tester-config: smoke-test/eslint-remote-tester.config.js
eslint-remote-tester-config: smoke-test/eslint-remote-tester.config.ts
Copy link

@AriPerkkio AriPerkkio Jan 2, 2022

Choose a reason for hiding this comment

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

@G-Rath this path is still incorrect 😄

-          eslint-remote-tester-config: smoke-test/eslint-remote-tester.config.ts
+          eslint-remote-tester-config: eslint-remote-tester.config.ts

#1002 (comment)

Edit: Right there's an open PR for this, great!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup

@AriPerkkio
Copy link

does the remote tester support anyway to handle linting repositories multiple times with different rule configurations?

At the moment, no. I've also had this idea before but didn't proceed with implementing it. I think the implementation wouldn't be that difficult though.
This is the spot where we are reading the config.eslintrc which can be either a single configuration, or function returning one. https://github.com/AriPerkkio/eslint-remote-tester/blob/30803c6b628da08515c6fbbe49065153dd727982/lib/engine/worker-task.ts#L212-L218
It could simply return a list of configuration which would then be looped in the rest of workerTask below.

Currently it is only possible to initialize config.eslintrc based on repository, e.g. checking whether project has tsconfig.json and enabling certain rules: https://github.com/AriPerkkio/eslint-remote-tester/blob/30803c6b628da08515c6fbbe49065153dd727982/ci/plugin-configs/typescript-eslint-eslint-plugin.config.js#L10.

@AriPerkkio
Copy link

AriPerkkio commented Jan 2, 2022

tbh I'm sort of on the fence about sticking with this now that I know you've actually been running the same workflow for our plugin and from what I can tell we've never actually had an error

I've been running the jest/all for long time and this has been one of the most stable plugin I've been testing. Though there have been some other plugins as well which have been running successfully for months and then suddenly caused random crashes. eslint-plugin-regexp is a good example here.

Running these tests in repository where main branch contains unreleased changes makes a lot sense, e.g. eslint-plugin-react. However this project uses automatic releasing so that's not perfect reasoning here.

I think you'll get the most from these tests if eslintrc.rules are configured extensively to cover as much code blocks as possible. This is something I've imagined to happen in projects themselves.
Also bringing the tests closer to the actual plugin projects is always better since I don't have to be building minimal repros and reporting the bugs anymore. 😄
You'll get issues reported by Github bot automatically in the project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants