Skip to content

(@wdio/cli): Support for sharding#11441

Merged
christian-bromann merged 5 commits intomainfrom
cb/sharding
Oct 17, 2023
Merged

(@wdio/cli): Support for sharding#11441
christian-bromann merged 5 commits intomainfrom
cb/sharding

Conversation

@christian-bromann
Copy link
Member

@christian-bromann christian-bromann commented Oct 16, 2023

Proposed changes

implements #11243

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

I will extend the docs once the feature is released with an example how to use sharding with Github Action.

Reviewers: @webdriverio/project-committers

@christian-bromann christian-bromann added the PR: New Feature 🚀 PRs that contain new features label Oct 16, 2023
const { total, current } = config.shard
if (total > 1) {
log.info(`No specs to execute in shard ${current}/${total}, exiting!`)
return resolve(0)
Copy link
Member

Choose a reason for hiding this comment

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

If we exit with code 1 when there are no tests, I would expect the same here

Copy link
Member Author

Choose a reason for hiding this comment

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

For sharding, this would be not desireable. For example: let's say a user has a CI setup that by defaults uses 10 workers and calls wdio always with --shard 1/10 ... --shard 10/10. Now let's say we have 100 test files and provide a dynamic parameter like --spec="SOME_PATTERN" which can resolves in 9 test files. In this case just because the dynamic spec parameter causes let's files to be selected than for what sharding is set up for should not cause the worker to break.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, I had not thought of it like that.

I'm assuming that the No specs found to run, exiting with failure with exit code 1 is still preferable but just to be just, I want to verify if this could also be resolved with an exit code of 0 then or if we actually want to fail here.

Since this is a question and it is not blocking, I'll resolve and approve 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Erwin!

Yes, the other case is desired because, e.g. someone has a typo in the spec pattern causing WebdriverIO no match no files. They would run it in CI not checking the logs and would be happy they always get green builds while in reality nothing gets tested. This is a case where we do want to fail the runner to make folks aware of that.

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.

Omg I really love this, clean code, well done on this Christian. Just one comment on the exit code 👍

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.

There's one merge conflict that needs to be resolved but other than that this LGTM 👍

const { total, current } = config.shard
if (total > 1) {
log.info(`No specs to execute in shard ${current}/${total}, exiting!`)
return resolve(0)
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, I had not thought of it like that.

I'm assuming that the No specs found to run, exiting with failure with exit code 1 is still preferable but just to be just, I want to verify if this could also be resolved with an exit code of 0 then or if we actually want to fail here.

Since this is a question and it is not blocking, I'll resolve and approve 👍

@christian-bromann christian-bromann merged commit a429320 into main Oct 17, 2023
@christian-bromann christian-bromann deleted the cb/sharding branch October 17, 2023 15:58
@kamal-kaur04
Copy link
Contributor

kamal-kaur04 commented Oct 27, 2023

@christian-bromann, was debugging an issue inside browserstack-wdio-service package wherein we used to send test status extending WDIO Reporter. We had some checks as per framework value inside the config. Now, it seems that the value isn't in the config object anymore. Possibly because we have removed framework and other few parameters from WDIO_DEFAULTS in the commit of this PR that is creating some test reporting failures. So, anyway to get this info inside onRunnerStart hook back?

Please note that it's happening for few test cases, mostly skipped ones.

@christian-bromann
Copy link
Member Author

Now, it seems that the value isn't in the config object anymore.

Which value?

So, anyway to get this info inside onRunnerStart hook back?

What information?

@kamal-kaur04
Copy link
Contributor

We had some checks as per framework value inside the config

^ this.config.framework

@christian-bromann
Copy link
Member Author

These information should be still accessible. Can you please point out which service or reporter hook used to have this information?

@kamal-kaur04
Copy link
Contributor

We are extending WDIOReporter - https://github.com/webdriverio/webdriverio/blob/main/packages/wdio-browserstack-service/src/reporter.ts

The info isn't available for certain cases, onRunnerStart doesn't contain config.framework when onTestSkip event is triggered.

@christian-bromann
Copy link
Member Author

Please create a separate issue for this. Any contributions are welcome!

@kamal-kaur04
Copy link
Contributor

@christian-bromann Sure, what was the reason of removing these keys from WDIO_DEFAULTS?

@christian-bromann
Copy link
Member Author

These are testrunner specific defaults and should not be a concern for the webdriverio package. They were moved to the CLI package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: New Feature 🚀 PRs that contain new features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants