Allow blob, tilde and env vars in app paths #386

sravanmedarapu · 2018-11-11T15:13:58Z

app, test and xctestrun-file values in Flank config should support below formats along with current formats.

tilda expansion(~)
environment variable expansion
globbing with * and **

Ex: 
app: ~/repo/app-*.apk
xctestrun-file: $DERIVED_DATA/**/*. xctestrun

Fix #336

codecov-io · 2018-11-11T15:20:58Z

Codecov Report

Merging #386 into master will increase coverage by 0.07%.
The diff coverage is 82.6%.

@@             Coverage Diff              @@
##             master     #386      +/-   ##
============================================
+ Coverage      79.9%   79.98%   +0.07%     
- Complexity      483      490       +7     
============================================
  Files            66       67       +1     
  Lines          1553     1594      +41     
  Branches        234      241       +7     
============================================
+ Hits           1241     1275      +34     
- Misses          173      177       +4     
- Partials        139      142       +3

Fixed failing tests Added unit tests

Add ArgsFileVisitor Fix paths

bootstraponline

JUnit coverage for edge cases in ArgsHelperFilePath (/**/, /*/, /tmp, etc.) will help improve confidence that the code is working as intended.

bootstraponline · 2018-11-11T19:52:09Z

test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt

@@ -45,6 +48,24 @@ object ArgsHelper {
        }
    }

+    fun evaluateFilePath(filePath: String): String {
+        var file = filePath.trim().replaceFirst("~", System.getProperty("user.home"))


~ should be replaced only if it's the first character in the string
https://stackoverflow.com/a/7163455

the regex should be ^~, we should add a JUnit test for this (for both the happy path and failure case).

~/tmp should be replaced
/tmp/~ should not be replaced

bootstraponline · 2018-11-11T19:57:30Z

test_runner/src/main/kotlin/ftl/args/ArgsFileVisitor.kt

+    fun walk(searchPath: Path): List<Path> {
+        val searchString = searchPath.toString()
+        // /Users/tmp/code/flank/test_app/** => /Users/tmp/code/flank/test_app/
+        val beforeGlob = Paths.get(searchString.substringBefore(RECURSE))


we probably need more testing around paths with /**/ and /*/

i'm not sure splitting by /**/ works for all cases

bootstraponline · 2018-11-11T19:59:37Z

test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt

+
+    // https://stackoverflow.com/a/2821201/2450315
+    private val envRegex = Pattern.compile("\\$([a-zA-Z_]+[a-zA-Z0-9_]*)")
+    private fun substituteEnvVars(text: String): String {


we should check this works for duplicate environment variables via more JUnit tests. There are probably other edge cases as well. For example the ${HOME} syntax which we should probably document as unsupported.

$HOME/$HOME/tmp

We should probably WARN but not fail for duplicate env variables. The warning is to notify the user that he's duplicated an env variable. Not failing, in case its intentional.

Regardless, I don't think we should handle custom env vars. That would likely be a pain to maintain.

bash doesn't warn on duplicates. I'm using bash as the model for the parsing logic.

doodla · 2018-11-12T16:08:40Z

test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt

+        val matcher = envRegex.matcher(text)
+        while (matcher.find()) {
+            val envName = matcher.group(1)
+            val envValue = System.getenv(envName) ?: ""


Should we raise an error/warning here? Silently ignoring un-found env variables seems like a bug.

this is standard behavior in Bash, that's how environment variables work sadly.

discussed on slack, printing a warning makes sense.

doodla · 2018-11-12T16:11:12Z

test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt

+        file = Paths.get(file).toAbsolutePath().normalize().toString()
+
+        // Avoid walking the folder's parent dir if we know it exists already.
+        if (File(file).exists()) return file


I would prefer if we also printed this value to console. Something like - Resolved app file path - "$file".
Under a conditional. if (filePath != file).

I think it should be printed already when the args are printed to stdout at the start of a test run.

# Conflicts: # test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt # test_runner/src/test/kotlin/ftl/args/AndroidArgsTest.kt # test_runner/src/test/kotlin/ftl/args/IosArgsTest.kt

bootstraponline · 2018-11-15T00:33:55Z

test_runner/src/test/kotlin/ftl/args/ArgsHelperTest.kt

+    }
+
+    @Test
+    fun tmpTest() {


This should be deleted.

sravanmedarapu added 3 commits November 11, 2018 10:10

Implemented evaluating paths Flank#336

db47575

Fixed failing tests

3cf9411

Added unit tests

73304dd

bootstraponline and others added 3 commits November 11, 2018 13:01

WIP path evaluation fixes

45559a7

Implemented evaluating paths Flank#336

ed6c617

Fixed failing tests Added unit tests

Path evaluation fixes

0ff99a3

Add ArgsFileVisitor Fix paths

bootstraponline force-pushed the issue-336 branch from f189eb1 to 0ff99a3 Compare November 11, 2018 19:28

Move file path tests to ArgsHelperFilePath

a841b81

bootstraponline suggested changes Nov 11, 2018

View reviewed changes

Update ArgsFileVisitor

51a2fa5

bootstraponline reviewed Nov 11, 2018

View reviewed changes

Remove unused environmentVariables rule

bb884d7

bootstraponline reviewed Nov 11, 2018

View reviewed changes

doodla reviewed Nov 12, 2018

View reviewed changes

Merge remote-tracking branch 'origin/issue-336' into issue-336

95fba21

# Conflicts: # test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt # test_runner/src/test/kotlin/ftl/args/AndroidArgsTest.kt # test_runner/src/test/kotlin/ftl/args/IosArgsTest.kt

bootstraponline reviewed Nov 15, 2018

View reviewed changes

test_runner/src/test/kotlin/ftl/args/ArgsHelperTest.kt Outdated

}

@Test

fun tmpTest() {

Copy link

Contributor

bootstraponline Nov 15, 2018

Choose a reason for hiding this comment

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

This should be deleted.

Added unit tests and fixed tilde replace regex.

7a4b6ec

sravanmedarapu force-pushed the issue-336 branch from de09c9b to 7a4b6ec Compare November 15, 2018 00:38

bootstraponline added 3 commits November 16, 2018 08:49

Merge branch 'master' into issue-336

aacf6bd

Fix tests

6211c0f

Fix single glob splitting

3dd27d4

bootstraponline force-pushed the issue-336 branch from ca4626c to 6a96663 Compare November 16, 2018 14:40

bootstraponline previously approved these changes Nov 16, 2018

View reviewed changes

bootstraponline dismissed their stale review via dbabf4d November 16, 2018 14:44

bootstraponline force-pushed the issue-336 branch from 6a96663 to dbabf4d Compare November 16, 2018 14:44

Update release notes

2eefeee

bootstraponline force-pushed the issue-336 branch from dbabf4d to 2eefeee Compare November 16, 2018 14:44

bootstraponline merged commit 25980b1 into Flank:master Nov 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow blob, tilde and env vars in app paths #386

Allow blob, tilde and env vars in app paths #386

sravanmedarapu commented Nov 11, 2018 •

edited by bootstraponline

Loading

codecov-io commented Nov 11, 2018 •

edited

Loading

bootstraponline left a comment •

edited

Loading

bootstraponline Nov 11, 2018

bootstraponline Nov 11, 2018

bootstraponline Nov 11, 2018

doodla Nov 12, 2018

bootstraponline Nov 12, 2018

doodla Nov 12, 2018

bootstraponline Nov 14, 2018

bootstraponline Nov 14, 2018

doodla Nov 12, 2018

bootstraponline Nov 12, 2018

bootstraponline Nov 15, 2018

Allow blob, tilde and env vars in app paths #386

Allow blob, tilde and env vars in app paths #386

Conversation

sravanmedarapu commented Nov 11, 2018 • edited by bootstraponline Loading

codecov-io commented Nov 11, 2018 • edited Loading

Codecov Report

bootstraponline left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sravanmedarapu commented Nov 11, 2018 •

edited by bootstraponline

Loading

codecov-io commented Nov 11, 2018 •

edited

Loading

bootstraponline left a comment •

edited

Loading