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

Add --shard-all and --shard-split options to test command #1955

Merged
merged 5 commits into from
Sep 3, 2024

Conversation

tokou
Copy link
Contributor

@tokou tokou commented Aug 27, 2024

Proposed changes

  • Add broadcast option to test command
  • This supersedes --shards but respects --device
  • It runs all the flows on all the available devices
  • This PR will be conflicting with Fix device selection #1953 quite a lot

Testing

  • Launch two devices and run maestro test -b on a folder with multiple flows
  • Make sure all flows run once on each device

Issues fixed

Fixes #1844

@bartekpacia
Copy link
Contributor

bartekpacia commented Aug 31, 2024

@tokou Which PR do you think we should merge first? This one or #1953?

@tokou
Copy link
Contributor Author

tokou commented Aug 31, 2024

Let's start with this one.

Copy link
Contributor

@bartekpacia bartekpacia left a comment

Choose a reason for hiding this comment

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

This LGTM, I'm happy to see such a useful change taking so little LoC! huge kudos:)

  • what do you mean by "--broadcast options supersedes --shards"? I'd prefer to not introduce breaking changes.

  • --broadcast sounds a bit confusing to me. I'd prefer sth different. Here are some ideas from chatgpt:

    Screenshot 2024-08-31 at 16 51 57

@tokou
Copy link
Contributor Author

tokou commented Aug 31, 2024

what do you mean by "--broadcast options supersedes --shards"? I'd prefer to not introduce breaking changes.

It's not breaking anything as it only affects cases where both --shards and --replicate are present (which can't already be the case)
In that case, as those are conflicting, we honor --replicate and ignore --shards

Here's a recap. Let's say we have 3 launched devices.

Arguments Effect
<no args> Prompts the user to choose 1 out of the 3 devices
--shards 2 Splits all tests across 2 out of the 3 devices
--replicate --shards 2 Runs all tests on all 3 devices
--device emulator-5554 Runs all tests on the specified device
--device emulator-5554 --shards 2 Runs all tests on the specified device
--device emulator-5554 --replicate Runs all tests on the specified device
--device emulator-5554 --replicate --shards 2 Runs all tests on the specified device

@bartekpacia
Copy link
Contributor

I see, this makes sense! Perhaps it'd be great to add this to the docs.

Also heads up that the E2E test is failing.

@bartekpacia
Copy link
Contributor

Test failing is false positive from #2005

Details Screenshot 2024-09-02 at 19 28 40

- Deprecate `--shards`
- Introduce `--shard-split` and `--shard-all`
@bartekpacia
Copy link
Contributor

bartekpacia commented Sep 3, 2024

Another false positive from #2005. They are getting more and more crazy:

Details Screenshot 2024-09-03 at 12 55 28

@bartekpacia bartekpacia merged commit 9ad6e39 into mobile-dev-inc:main Sep 3, 2024
5 of 6 checks passed
@tokou tokou deleted the broadcast-tests branch September 3, 2024 11:55
@bartekpacia bartekpacia changed the title Add broadcast option to test command Add --shard-all and --shard-split options to test command Sep 12, 2024
Copy link

@simakovroman23 simakovroman23 left a comment

Choose a reason for hiding this comment

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

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

Successfully merging this pull request may close these issues.

Add possibility to test in parallel without sharding (run the whole test on every device)
3 participants