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

chore: add npm script to run WebdriverIO test #18238

Merged
merged 3 commits into from Mar 29, 2024
Merged

Conversation

fasttime
Copy link
Member

@fasttime fasttime commented Mar 28, 2024

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[X] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Re-enabled WebdriverIO tests. Added npm script test:wdio to run the WebdriverIO test locally.

Is there anything you'd like reviewers to focus on?

WebdriverIO has been running smoothly in CI for the last couple of months. Shall we try to re-enable it for tests in Jenkins? Or maybe just for local tests?

@eslint-github-bot eslint-github-bot bot added the chore This change is not user-facing label Mar 28, 2024
Copy link

netlify bot commented Mar 28, 2024

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 9b57f7e
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/66074a2c0390a5000842d6be

@fasttime fasttime changed the title chore: re-enable WebdriverIO broswer tests chore: re-enable WebdriverIO browser tests Mar 28, 2024
@fasttime fasttime marked this pull request as ready for review March 28, 2024 06:30
@fasttime fasttime requested a review from a team as a code owner March 28, 2024 06:30
@aladdin-add
Copy link
Member

it was disabled as encountered the error: Command failed: which google-chrome

#17572 (comment)

Is there any way we can verify that it has been fixed?

@fasttime
Copy link
Member Author

it was disabled as encountered the error: Command failed: which google-chrome

#17572 (comment)

Is there any way we can verify that it has been fixed?

Hm, good point. I don't know how this can be verified. But if it's not necessary to run the browser tests in Jenkins maybe we could add a switch to the task in Makefile.js, so the browser test will run locally with npm test?

@mdjermanovic
Copy link
Member

I think it's fine not to run the browser test on Jenkins, as we'll always have CI checks right before the release (when @eslnt/js changes are committed).

I also think it's fine to keep the browser test excluded from npm test because it rarely happens that a change breaks these tests and it would eventually be caught in CI anyway. We could add a package.json script that runs the browser test so that if the test happens to fail in CI, the contributor can easily run it locally.

@nzakas
Copy link
Member

nzakas commented Mar 28, 2024

Yeah, we disabled these on Jenkins because there always seemed to be some configuration issue and we already knew that GitHub CI passed with it running.

I also agree with @mdjermanovic that we don't need to add it to npm test. That command already take a long time to complete, and the browser use case is an edge case that we can watch for during CI.

@fasttime fasttime changed the title chore: re-enable WebdriverIO browser tests chore: add npm script to run WebdriverIO test Mar 29, 2024
@fasttime
Copy link
Member Author

I also think it's fine to keep the browser test excluded from npm test because it rarely happens that a change breaks these tests and it would eventually be caught in CI anyway. We could add a package.json script that runs the browser test so that if the test happens to fail in CI, the contributor can easily run it locally.

Thanks! I've re-excluded the browser test from the general test task and added an npm script to run it manually.

package.json Outdated
@@ -32,7 +32,8 @@
"test": "node Makefile.js test",
"test:cli": "mocha",
"test:fuzz": "node Makefile.js fuzz",
"test:performance": "node Makefile.js perf"
"test:performance": "node Makefile.js perf",
"test:wdio": "node Makefile.js wdio"
Copy link
Member

Choose a reason for hiding this comment

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

wido is just the used tool, and the gha job name is test-on-broswer. :)

Suggested change
"test:wdio": "node Makefile.js wdio"
"test:browser": "node Makefile.js wdio"

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 9b57f7e, thanks.

@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Mar 29, 2024
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit a98babc into main Mar 29, 2024
18 checks passed
@mdjermanovic mdjermanovic deleted the re-enable-wdio branch March 29, 2024 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion chore This change is not user-facing
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

None yet

4 participants