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

Js log dollar sign #2060

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Js log dollar sign #2060

wants to merge 3 commits into from

Conversation

tokou
Copy link
Contributor

@tokou tokou commented Sep 16, 2024

Proposed changes

  • I updated the regexes in fun String.evaluateScripts(jsEngine: JsEngine) to not prevent the script to have any dollar sign
  • I'm confident this fixes the linked issue
  • I'm not confident enough this does not brake on a weird edge case
  • I tried thinking of some JS content that contains a dollar sign and that would brake due to this change but I don't have any idea. Maybe you'll find one
  • I added forwarding of the js console to maestro log file
  • I added a MAESTRO_CLI_LOG_LEVEL env var to limit log output level if needed

Testing

  • Added unit test
  • Tested with following e2e file
appId: com.example.example
---
- launchApp
- evalScript: ${console.log('Hello!')}
- evalScript: ${console.log('Hello money! $')}
- evalScript: ${console.log('Hello money! \$')}
- evalScript: ${console.log('$')}
- evalScript: ${console.log('\$')}
 ║
 ║  > Flow: dollar
 ║
 ║    ✅   Launch app "com.example.example"
 ║    ✅   Run ${console.log('Hello!')}
 ║         Log messages:
 ║           Hello!
 ║    ✅   Run ${console.log('Hello money! $')}
 ║         Log messages:
 ║           Hello money! $
 ║    ✅   Run ${console.log('Hello money! \$')}
 ║         Log messages:
 ║           Hello money! $
 ║    ✅   Run ${console.log('$')}
 ║         Log messages:
 ║           $
 ║    ✅   Run ${console.log('\$')}
 ║         Log messages:
 ║           $
 ║

Issues fixed

Fixes #1760

Fixes #1744

@@ -8,7 +8,7 @@ import maestro.orchestra.MaestroCommand
object Env {

fun String.evaluateScripts(jsEngine: JsEngine): String {
val result = "(?<!\\\\)\\\$\\{([^\$]*)}".toRegex()
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder what the original author intended with the original regex here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I believe it was to detect anything that's encosed in ${} and that does not contain $ inside

Copy link
Contributor

Choose a reason for hiding this comment

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

But why would they have done specific work to exclude the dollar, if it were as simple as just .*? ?
Is there an additional risk we've not considered?

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