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

Allow env values in setLocation and travel commands #1769

Closed
wants to merge 0 commits into from

Conversation

prasanta-biswas
Copy link
Contributor

@prasanta-biswas prasanta-biswas commented Jun 18, 2024

Proposed Changes

Fixes an issue where setLocation command fails if latitude and longitude values are passed from environment variables.

Testing

  • Ran integration tests using ./gradlew :maestro-test:test, all tests passed.
  • Manually tested setLocation command by passing latitude and longitude as env parameters from the command line and within the flow file using the env keyword. It worked for both inline-defined latitude and longitude and passed from env.

Issues Fixed

fixes #1765

@Alaksion
Copy link

@prasanta-biswas thank you! I was about to open a PR solving this exact same issue

@@ -33,7 +33,7 @@ internal class MaestroCommandTest {
@Test
fun `description (with a label)`() {
// given
val maestroCommand = MaestroCommand(SetLocationCommand(12.5266, 78.2150, "Set Location to Test Laboratory"))
val maestroCommand = MaestroCommand(SetLocationCommand("12.5266", "78.2150", "Set Location to Test Laboratory"))

Choose a reason for hiding this comment

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

Can you write a test using negative coordinates ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Alaksion done. Please review.

Choose a reason for hiding this comment

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

thanks!

@lucaswiechmann lucaswiechmann self-requested a review June 18, 2024 20:21
@lucaswiechmann
Copy link

Hello @prasanta-biswas
Build is failing. Could you please try running build and installDist and make sure they are passing?

  • ./gradlew installDist
  • ./gradlew clean build
* What went wrong:
Execution failed for task ':didb:compileKotlin'.
> A failure occurred while executing org.jetbrains.kotlin.compilerRunner.GradleCompilerRunnerWithWorkers$GradleKotlinCompilerWorkAction
   > Compilation error. See log for more details

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.

@prasanta-biswas

This comment was marked as off-topic.

@prasanta-biswas
Copy link
Contributor Author

@lucaswiechmann possible to merge this?

@bartekpacia bartekpacia changed the title Allow env values in setLocation command Allow env values in setLocation and travel commands Jul 10, 2024
@lucaswiechmann
Copy link

Hey @prasanta-biswas
This PR is breaking things internally. @bartekpacia and I is taking a close look on it this week!

@Alaksion
Copy link

@lucaswiechmann any updates on this?

@bartekpacia
Copy link
Member

Hey @Alaksion, sorry about the delay! I plan to get to it this week. Thanks for understanding.

@prasanta-biswas
Copy link
Contributor Author

@bartekpacia any update here?

@bartekpacia
Copy link
Member

bartekpacia commented Aug 6, 2024

Hey @prasanta-biswas, could you rebase/merge with master?

@prasanta-biswas
Copy link
Contributor Author

@bartekpacia done

@prasanta-biswas
Copy link
Contributor Author

@bartekpacia test cloud job failed due to incorrect command. could you please check the job configuration?

@bartekpacia
Copy link
Member

Don't worry about it, it's because the secret is missing

@prasanta-biswas
Copy link
Contributor Author

@bartekpacia can you please retry the job if the missing secret is added?

@bartekpacia
Copy link
Member

Hey @prasanta-biswas, don't worry about the secret, it's not available to jobs running for PRs open-source contributors. We want to guard that secret:)

Copy link
Member

@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.

The added test doesn't actually test the newly added feature. We need a test like this:

appId: com.example.app
---
setLocation:
  latitude: ${LATITUDE}
  longitude: ${LONGITUDE`

and then somehow mock those env vars

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.

Failing to parse env values for setLocation command
4 participants