Introduce ShellExecutor, add tests for GitKtx#105
Conversation
davidbilik
force-pushed
the
feature/shell-executor
branch
3 times, most recently
from
b2f9fcb to
9ca70e1
Compare
|
|
||
| return process.inputStream.bufferedReader().readText() | ||
| fun exec(command: String, arguments: List<String> = emptyList()): String { | ||
| return ShellExecutorFactory.get().execute(command, arguments) |
There was a problem hiding this comment.
i don't really like the idea of proxy here, we can directly use utils
There was a problem hiding this comment.
well that was what we discussed with @f-meloni in my previous PR 🤔
| fun execute(command: String, arguments: List<String> = emptyList()): String | ||
| } | ||
|
|
||
| class ShellExecutorImpl : ShellExecutor { |
There was a problem hiding this comment.
the ShellExecutorImpl with related factory can be deleted then
| */ | ||
| internal class GitKtxTest { | ||
|
|
||
| class MockShellExecutor : ShellExecutor { |
There was a problem hiding this comment.
what about using a mock library? i was planning to add mockk.. let's use that
There was a problem hiding this comment.
ok, I personally don't use mock libraries, I prefer fakes, but in this context it's probably better.
f-meloni
left a comment
f-meloni
left a comment
There was a problem hiding this comment.
I personally prefer this implementation.
Utils is an object that is created to help who uses danger and does too many things to be used internally where I would like instead some composition internally
danger-kotlin-library/src/test/kotlin/systems/danger/kotlin/GitKtxTest.kt
Outdated
Show resolved
Hide resolved
|
Btw this PR also fixes an issue with swapped additions/deletions computation in changed lines. I've discovered that during tests, which made me feel good that the tests were useful. But on the other hand now is version 0.6.0 released that contains this bug :( |
danger-kotlin-library/src/test/kotlin/systems/danger/kotlin/GitKtxTest.kt
Outdated
Show resolved
Hide resolved
|
So what do you guys suggest to do with this PR? |
I’m happy to see this merged, is great, we just need to fix the indentation and the merge conflict :) |
Introduce abstract ShellExecutor with one specific implementation used in production code. This is useful for tests when we want to isolate our test from actual shell commands. Use this abstraction in tests for Git changed lines extensions.
davidbilik
force-pushed
the
feature/shell-executor
branch
from
e2fa634 to
4c3f695
Compare
Ok then, conflict resolved, indentation fixed and I've added mockK to the project and used that in my test |
I preferred it without mockK to be fair, I don't think it added much value here, but is not a good reason to hold this PR, we can discuss this separately |
Well @gianluz suggested using mock library and you did not comment on that review so I thought that you don't have anything against that 🤷 your review of this PR were kind of clashing with each other so I don't know what should I work on and what not |
We don't always agree on everything, otherwise we would be the same person :D |
3 participants
Introduce abstract ShellExecutor with one specific implementation
used in production code. This is useful for tests when we want to
isolate our test from actual shell commands. Use this abstraction in
tests for Git changed lines extensions.