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 device selection #1953

Merged
merged 17 commits into from
Sep 5, 2024
Merged

Conversation

tokou
Copy link
Contributor

@tokou tokou commented Aug 27, 2024

Proposed changes

  • The issue seems to be in the logic related to selecting the devices depending on the shards
  • I tried cleaning it up as best as I could but that part of the code is kinda all over the place
  • I started working on launching existing devices in addition to creating new ones for shards but I wonder what's the value from such a feature

Testing

  • Launch 2 devices
  • Run a folder with multiple flows without specifying sharding
  • Run a folder with multiple flows with shards = 2
  • Run a folder with multiple flows with shards = 3

Issues fixed

Fixes #1849

@tokou tokou force-pushed the fix-device-selection branch from e278c70 to 2a4692a Compare August 30, 2024 22:21
@bartekpacia
Copy link
Contributor

Another false positive of #2005...

Screenshot 2024-09-03 at 12 02 48

# Conflicts:
#	maestro-cli/src/main/java/maestro/cli/command/TestCommand.kt
@bartekpacia
Copy link
Contributor

I tested this and it seems the issue is not fixed, i.e. Maestro still doesn't ask what device to run on when more than 1 device is connected.

To reproduce, start 2 android emulators, and then:

./maestro-cli/build/install/maestro/bin/maestro test --include-tags passing,failing --exclude-tags ios e2e/workspaces/demo_app

@bartekpacia
Copy link
Contributor

Aside from that it looks good. Also thanks a ton for resolving all merge conflicts!

Verified

This commit was signed with the committer’s verified signature.
bartekpacia Bartek Pacia
@bartekpacia
Copy link
Contributor

Ugh, more conflicts after I merged #1995

# Conflicts:
#	maestro-cli/src/main/java/maestro/cli/command/TestCommand.kt
@bartekpacia
Copy link
Contributor

I started working on launching existing devices in addition to creating new ones for shards but I wonder what's the value from such a feature

I think it's OK to not do this.

Actually, I don't like that Maestro CLI can create new devices as part of maestro test. In my opinion, to keep code cleaner and responsibilities separate, maestro test should merely print "Not enough devices. Start some or create with maestro start-device" instead of offering the user to go through the device creation flow.

@tokou
Copy link
Contributor Author

tokou commented Sep 4, 2024

Yeah I fully agree
But the feature was present that's why I kept it. I'll gladly get rid of that part of the code

@tokou
Copy link
Contributor Author

tokou commented Sep 4, 2024

@bartekpacia I got rid of device creation. I also switched --shard-all to a boolean as it makes more sense to me like that. Let me know what you think.

@bartekpacia
Copy link
Contributor

bartekpacia commented Sep 4, 2024

This is awesome. Thank you.

Re: --shard-all – I prefer to keep it as an int arg, for (1) consistency with --shard-split, and also I think it's just useful to expose it to users and doesn't cost us much.

tokou and others added 2 commits September 4, 2024 21:53
This reverts commit 9238f97.

Verified

This commit was signed with the committer’s verified signature.
bartekpacia Bartek Pacia
@bartekpacia bartekpacia merged commit 2d9e106 into mobile-dev-inc:main Sep 5, 2024
6 checks passed
@tokou tokou deleted the fix-device-selection branch September 5, 2024 16:57
@DavidREntwistle
Copy link

DavidREntwistle commented Oct 16, 2024

This fix doesn't look to have resolved issue in CLI v. 1.39.0.
I've re-raise this issue now here: #2096

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.

Maestro stopped asking which device to use when many are running, defaults to the first one
3 participants