add more functions to file module #262
Conversation
Signed-off-by: drago8888 <rjmateus1985@gmail.com>
Signed-off-by: drago8888 <rjmateus1985@gmail.com>
|
Challenge from Ricardo Mateus |
There was a problem hiding this comment.
Hey @drago8888, thanks for your contributions! Your patch looks good to me, just please consider those two comments I added before we can merge it.
| args.put("src", src); | ||
| args.put("path", path); | ||
| return new LocalCall<>("file.symlink", Optional.empty(), Optional.of(args) | ||
| , new TypeToken<Boolean>(){}); |
There was a problem hiding this comment.
This comma should probably be on the previous line?
| new MinionList("minion1"), AUTH).toCompletableFuture().join(); | ||
| assertTrue(response.get("minion1").result().get()); | ||
|
|
||
| // false response |
There was a problem hiding this comment.
For all these tests I would suggest to separate the tests for the true and false responses, and rather create one test method for each use-case. This ensures that all assertions are actually performed. Otherwise if the first assertion is failing, the second one would not even be executed. Also if this test for example would fail, you don't see directly which of the use-cases is broken (the true or the false response).
There was a problem hiding this comment.
I agree with you, I think each case should be in a separated method and that was my first approach.
The reason behind my change of approach was to keep the consistency with the rest of the project test, with one test method per function with several scenarios.
However in this case following the right way instead of consistency seems not to bring any harm to the future of the project, therefore I will split all the tests into suggested into one per scenario.
Signed-off-by: drago8888 <rjmateus1985@gmail.com>
drago8888
force-pushed
the
fileModuleFunctions
branch
from
2ff4f79
to
e8eedf9
Compare
Add support for the following functions:
Signed-off-by: drago8888 rjmateus1985@gmail.com