#16: New Engine: GraphvizCmdLineEngine

[kerkhofsd]

No description provided.

[kerkhofsd]

@nidi3 I created a test for my GraphvizCmdLine engine. Since the dot command is not available on the test executor, this tests fails. What do you suggest as a solution for this problem?

[nidi3]

Thinks for the PR, good work.

Just a few comments:

• Would it be possible to just copy a simple mock "dot" file (.bat or .sh) on the filesystem $PATH in order to fix the test?
• Are the 2 apache commons dependencies really needed?
• You introduced a circular dependency between engine and service packages: engine.mayUse(model, service, executor); and service.mayUse(engine, executor);

[kerkhofsd]

Hi @nidi3

• Would it be possible to just copy a simple mock "dot" file (.bat or .sh) on the filesystem $PATH in order to fix the test?
  I created a solution where I mock the CommandExecutor. Hence for the tests, the dot command get's executed by the mock, that just copies the expected output file to the correct folder.
  What do you think about this solution?
• Are the 2 apache commons dependencies really needed?
  The commons-lang3 was needed to determine the OS. I removed this dependency.
  The commons-exec is quite essential in the code to execute the Commands...
• You introduced a circular dependency between engine and service packages: engine.mayUse(model, service, executor); and service.mayUse(engine, executor)
  Completely agree, I fixed this.

PR Updated, feel free to share your thoughts / comments!

[codecov-io]

Codecov Report

Merging #19 into master will decrease coverage by 0.41%.
The diff coverage is 71.81%.

@@             Coverage Diff              @@
##             master      #19      +/-   ##
============================================
- Coverage     76.78%   76.37%   -0.42%     
- Complexity      510      556      +46     
============================================
  Files            46       51       +5     
  Lines          1650     1799     +149     
  Branches        177      189      +12     
============================================
+ Hits           1267     1374     +107     
- Misses          292      320      +28     
- Partials         91      105      +14

Impacted Files	Coverage Δ	Complexity Δ
...n/java/guru/nidi/graphviz/service/SystemUtils.java	50% <50%> (ø)	5 <5> (?)
...ava/guru/nidi/graphviz/service/CommandBuilder.java	65.62% <65.62%> (ø)	9 <9> (?)
...ru/nidi/graphviz/engine/GraphvizCmdLineEngine.java	72.72% <72.72%> (ø)	12 <12> (?)
...java/guru/nidi/graphviz/service/CommandRunner.java	77.14% <77.14%> (ø)	17 <17> (?)
...va/guru/nidi/graphviz/service/DefaultExecutor.java	86.66% <86.66%> (ø)	3 <3> (?)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0d0441...3ed8199. Read the comment docs.

[nidi3]

Sorry for the late answer, here are some more things:

• VizJsOptions are not considered, right?
• Is ICommandExecutor really needed, there's only one implementation. If yes, please drop the I
• I see System.outs, so I'm going to setup proper logging

[nidi3]

logging is in place.

[kerkhofsd]

Hi @nidi3, thans for the feedback

• VizJsOptions are not considered, right?
  Indeed. But with some parsing of the VizJsOptions to the correct command line arguments, this should be possible.
• Is ICommandExecutor really needed, there's only one implementation. If yes, please drop the I
  You are right, in this case it is not needed
• I see System.outs, so I'm going to setup proper logging
  I used the logging you provided!

[nidi3]

Sounds great. I refactored AbstractGraphvizEngine so that the signature is String doExecute(String src, Options options); and there's no need to parse the js call.

[kerkhofsd]

PR Updated, Options are now included!

[nidi3]

Great, going to merge it.

[nidi3]

It's merged. I adjusted some code styling and replaced all System.out and e.printStacktrace() with logger calls.
I think the handling of Options.engine is not quite right, going to fix this.

[nidi3]

Sorry, Options.engine works perfectly.

[kerkhofsd]

Thanks for merging my PR ;-)