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.

Sign up for GitHub

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
+25 −3
Open

Js log dollar sign #2060

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2eb0b22
Log js console
tokou Sep 16, 2024
ff99a94
Add log level config env var
tokou Sep 16, 2024
743044c
Fix script detection regexes
tokou Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion maestro-client/src/main/java/maestro/debuglog/LogConfig.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@ object LogConfig {

private const val DEFAULT_FILE_LOG_PATTERN = "%d{HH:mm:ss.SSS} [%5level] %logger.%method: %msg%n"
private const val DEFAULT_CONSOLE_LOG_PATTERN = "%highlight([%5level]) %msg%n"
private const val DEFAULT_LOG_LEVEL = "ALL"

private val FILE_LOG_PATTERN: String = System.getenv("MAESTRO_CLI_LOG_PATTERN_FILE") ?: DEFAULT_FILE_LOG_PATTERN
private val CONSOLE_LOG_PATTERN: String = System.getenv("MAESTRO_CLI_LOG_PATTERN_CONSOLE") ?: DEFAULT_CONSOLE_LOG_PATTERN
private val LOG_LEVEL: String = System.getenv("MAESTRO_CLI_LOG_LEVEL") ?: DEFAULT_LOG_LEVEL

fun configure(logFileName: String, printToConsole: Boolean) {
val loggerContext = LoggerFactory.getILoggerFactory() as LoggerContext
Expand All @@ -27,7 +29,7 @@ object LogConfig {
createAndAddConsoleAppender(loggerContext)
}

loggerContext.getLogger("ROOT").level = Level.ALL
loggerContext.getLogger("ROOT").level = Level.toLevel(LOG_LEVEL, Level.ALL)
}

private fun createAndAddConsoleAppender(loggerContext: LoggerContext) {
Expand Down
4 changes: 2 additions & 2 deletions maestro-orchestra-models/src/main/java/maestro/orchestra/util/Env.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?

val result = "(?<!\\\\)\\\$\\{(.*?)}".toRegex()
.replace(this) { match ->
val script = match.groups[1]?.value ?: ""

Expand All @@ -20,7 +20,7 @@ object Env {
}

return result
.replace("\\\\\\\$\\{([^\$]*)}".toRegex()) { match ->
.replace("\\\\\\\$\\{(.*?)}".toRegex()) { match ->
match.value.substringAfter('\\')
}
}
Expand Down
18 changes: 18 additions & 0 deletions maestro-orchestra-models/src/test/kotlin/maestro/orchestra/util/EnvTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@ package maestro.orchestra.util
import com.google.common.truth.Truth.assertThat
import java.io.File
import kotlin.random.Random
import maestro.js.GraalJsEngine
import maestro.orchestra.ApplyConfigurationCommand
import maestro.orchestra.DefineVariablesCommand
import maestro.orchestra.MaestroCommand
import maestro.orchestra.MaestroConfig
import maestro.orchestra.util.Env.evaluateScripts
import maestro.orchestra.util.Env.withDefaultEnvVars
import maestro.orchestra.util.Env.withEnv
import maestro.orchestra.util.Env.withInjectedShellEnvVars
Expand Down Expand Up @@ -60,4 +62,20 @@ class EnvTest {

assertThat(withEnv).containsExactly(defineVariables, applyConfig)
}

@Test
fun `evaluateScripts regex`() {
val engine = GraalJsEngine()
val inputs = listOf(
"${'$'}{console.log('Hello!')}",
"${'$'}{console.log('Hello Money! $')}",
"${'$'}{console.log('$')}",
)

val evaluated = inputs.map { it.evaluateScripts(engine) }

// "undefined" is the expected output when evaluating console.log successfully
assertThat(evaluated).containsExactly("undefined", "undefined", "undefined")
}

}
2 changes: 2 additions & 0 deletions maestro-orchestra/src/main/java/maestro/orchestra/Orchestra.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ class Orchestra(
command,
metadata.copy(logMessages = metadata.logMessages + msg)
)
jsLogger.info(msg)
}

val evaluatedCommand = command.evaluateScripts(jsEngine)
Expand Down Expand Up @@ -1386,5 +1387,6 @@ class Orchestra(
private const val MAX_ERASE_CHARACTERS = 50
private const val MAX_RETRIES_ALLOWED = 3
private val logger = LoggerFactory.getLogger(Orchestra::class.java)
private val jsLogger = LoggerFactory.getLogger("maestro.js.JsEngine.console.log")
}
}
Loading