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

Fix filtering test specs by part of their name using CLI --spec arg #11771

Conversation

tech-dm-klymenko
Copy link
Contributor

@tech-dm-klymenko tech-dm-klymenko commented Dec 1, 2023

Proposed changes

Issue: #9495
A bugfix for this functionality has been released:

If the --spec value does not point to a particular spec file, it is instead used to filter the spec filenames defined in your configuration.

To run all specs with the word “dialog” in the spec file names, you could use:

wdio wdio.conf.js --spec dialog

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Checklist

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have added proper type definitions for new commands (if appropriate)

Further comments

Reviewers: @webdriverio/project-committers

Copy link

linux-foundation-easycla bot commented Dec 1, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@erwinheitzman erwinheitzman left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, awesome work!

Could you add some tests to verify the behavior? 👍

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Thanks for taking a stab at it. I made some comments to hopefully simplify the code. Let me know if you have any questions.

packages/wdio-config/src/node/ConfigParser.ts Outdated Show resolved Hide resolved
packages/wdio-config/src/node/ConfigParser.ts Outdated Show resolved Hide resolved
packages/wdio-config/src/node/ConfigParser.ts Outdated Show resolved Hide resolved
packages/wdio-config/src/node/ConfigParser.ts Outdated Show resolved Hide resolved
@tech-dm-klymenko
Copy link
Contributor Author

@christian-bromann @erwinheitzman

Thank you for your comments!

Upon further consideration, I revisited the current approach and made adjustments. From my analysis, the main issues stemmed from two points:

  1. The setFilePathToFilterOptions method consistently used a glob pattern search. While effective when specifying the spec path to the --spec, it failed when we pass a keyword as --spec argument.

  2. During the initialization stage of the ConfigParser object, the merge method was invoked within its constructor. This, in turn, attempted to obtain paths to the specs through the setFilePathToFilterOptions method. However, at this stage, we lacked a list of specs from the config file, leading to an error.

I fixed issues above and now the --spec option works correctly. I have also included smoke tests to verify this functionality.

Please check my PR one more time

Copy link
Member

@erwinheitzman erwinheitzman left a comment

Choose a reason for hiding this comment

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

Added some feedback

@tech-dm-klymenko
Copy link
Contributor Author

@christian-bromann @erwinheitzman
I made changes according to your comments

@christian-bromann
Copy link
Member

@tech-dm-klymenko thanks! It seems that there are some issues in CI 🤔

@tech-dm-klymenko
Copy link
Contributor Author

@christian-bromann
Could you, please, tell me, do you have any other comments regarding the fix without taking into account the tests? Probably I’ll just add the tests as the separate PR later. And current implementation of tests will be reverted to avoid issues in CI

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

I think it makes sense to fix the test, it seems like we are very close

tests/tests-cli-spec-arg/wdio-with-all-passed.conf.js Outdated Show resolved Hide resolved
tests/tests-cli-spec-arg/wdio-with-failed.conf.js Outdated Show resolved Hide resolved
Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

I think there is another issue at hand here which is outside of the scope of your PR, I made a suggestion how to work around it.

tests/tests-cli-spec-arg/wdio-with-all-passed.conf.js Outdated Show resolved Hide resolved
Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Awesome, thanks a lot! 👍

@christian-bromann christian-bromann added the PR: Bug Fix 🐛 PRs that contain bug fixes label Dec 20, 2023
@christian-bromann
Copy link
Member

Fantastic first contribution 🎉

@christian-bromann christian-bromann merged commit d7e4b20 into webdriverio:main Dec 20, 2023
8 checks passed
@christian-bromann christian-bromann added the First Contribution 🎉 Highlights PRs of people that made their first contribution to the project label Dec 20, 2023
@tech-dm-klymenko
Copy link
Contributor Author

Thank you very much for your appreciation! 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First Contribution 🎉 Highlights PRs of people that made their first contribution to the project PR: Bug Fix 🐛 PRs that contain bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants