support deleting data volumes while deleting services

[ilgooz]

closes #422

@mesg-foundation/core Docker creates one more extra data volume which we cannot track it's creation so we cannot delete it. I don't know what it's for. We need to investigate it. I'm putting steps to reproduce.

First delete all of your data volumes for a clean inspection:

• run docker volume rm $(docker volume ls -q --filter dangling=true)

Following has no problems:

• run ./dev-cli service deploy https://github.com/ilgooz/service-articles
• run docker volume ls > You'll see one.
• run ./dev-cli service delete c13a74b6b50f19791de1ba3ab561928c71376882
• run docker volume ls > You'll see all volumes gone.

Problematic one:

• run ./dev-cli service deploy https://github.com/ilgooz/service-articles
• run ./dev-cli service start c13a74b6b50f19791de1ba3ab561928c71376882
• run docker volume ls > You'll see two.
• run ./dev-cli service delete c13a74b6b50f19791de1ba3ab561928c71376882
• run docker volume ls > You'll see one gone but extra volume left.

asc:core ilgooz$ docker volume ls
DRIVER              VOLUME NAME
asc:core ilgooz$ docker volume ls
DRIVER              VOLUME NAME
local               41030f6ecde6ef18ca7b66fb341351e1b950acf8
local               d2161474f2c18d96ef0a067fabd29f84e38485f4b71f70ba9acebe97926c0420
asc:core ilgooz$ docker volume ls
DRIVER              VOLUME NAME
local               d2161474f2c18d96ef0a067fabd29f84e38485f4b71f70ba9acebe97926c0420
asc:core ilgooz$ 

[antho1404]

General feeling: We talk about volumes, data, persistent data, from the user perspective and in the variable names, we might want to have some consistency. I would use volumes everywhere or persistent data, but we talk about data for the events and task, it can be confusing to use persistent data

[ilgooz]

@antho1404

General feeling: We talk about volumes, data, persistent data, from the user perspective and in the variable names, we might want to have some consistency. I would use volumes everywhere or persistent data, but we talk about data for the events and task, it can be confusing to use persistent data

In the users' perspective I think we should say persistent data because volume name is a bit technical. For internal use, data volume/volume name makes for sense because we're not deleting or creating data, we're creating data volumes or deleting data volumes.

Do you have any suggestions?

[ilgooz]

How about -delete-data-volumes?

[NicolasMahe]

How about -delete-data-volumes?

I like --delete-data. From a user point of view, he doesn't know about docker volume. He only cares about data.

[ilgooz]

This test doesn't cover the tests deleted within this commit. I don't understand why they're removed.

[krhubert]

It tests only runE method. The previous tests were for both preRunE and runE at the same time

[ilgooz]

we should have tests for survey. we already had some tests, why removing them within this commit?

[krhubert]

Because it was too complicated,

Again there was a new interface, new mock, mocks in the tests just to test AskOne method. We need to find a better way to test it. If we will mock everything that we can't tests we will end up in mock hell.

Also, those tests mix testing of validation (preRunE) and execution (runE) and good unit tests should test only one behavior/func at once (test one thing at a time).

[NicolasMahe]

On survey's README, the guy suggest to use go-expect to catch stuff in stdout

[ilgooz]

I don't approve removing the tests for survey. Having one more mock doesn't lead to a mock hell. I had a look on suvery's testing utilities yesterday but it seemed complicated to me, mocks are way simpler. If you want to delete the tests anyway please put their replacement otherwise we will lose some meaningful tests.

[NicolasMahe]

https://github.com/mesg-foundation/core/issues/622

[krhubert]

My motiviation to delete those tests was:

• mock hell what I mean by that if we swtich to passing interface to commands we will need: daemon.Deamon, api.API, service.Service (maybe) CoreClient mocks so it's already a lot. Adding one more mock seems good from short perspective, but in long run it's not so obious.

• so choosing mocking should be the last thing while testing. We alwasy need to try to find other solutions/design patterns that will free code of mocks. Why I think so. Look at go source code:
  There is only one mock net/dnsclient_unix_test.go:func here.

➜  moby git:(master) gogrep -i mock |grep struct |wc -l
36

So docker has 36 mocks and we probably haven't reached 2% of their codebase and we already have:

➜  core git:(feature/auto-remove-volume) ✗ gogrep -i mock |grep struct |wc -l
13

13 mocks.

• while mocking seems easy to go solution it is not in long run (compared to js where it's standard and it's super easy, but js are more flexible as it is dynamic programming language)

• survey suggested go-expect and vx10 for testing but vx10 is suspicious to me, but it's still better to use it as it doesn't require to introduce changes in the source code.

• about using https://github.com/AlecAivazis/survey/blob/v1.7.0/confirm_test.go#L21 I was wrong. It can't be used.

So to summarize we should think of a better solution if possible, sleep with it, look from a different perspective, but introduce mocks because it easy should be avoided in general.

In was the same with commands package before caputereStd was created, there were no tests at all and in the meantime, we introduced them because we find a way to do so.

[ilgooz]

Again, this is something that I just don't agree. There is no logical reason to remove a test if it's not replaced by an equivalent one. I'm not even talking about using the mocks here, it's not relevant.

Survey pkg doesn't provide a nice way of testing as I already checked so, it's a proper option to use mocks here. I don't mind adopting a better way. Could you please provide one? It seems that we're seeing things in different perspectives.

mock hell what I mean by that if we swtich to passing interface to commands we will need: daemon.Deamon, api.API, service.Service (maybe) CoreClient mocks so it's already a lot. Adding one more mock seems good from short perspective, but in long run it's not so obvious.

This is totally an another topic. If we want to reduce possibility for programming errors we should have tests about how daemon and coreapi used in the commands/* pkgs as well. And I'd like doing that with unit tests. And I remember that you said it's not necessary to test wrapper funcs (the default implementation between these pkgs). And we don't have tests for them. I also suggested to have at least some integration tests for reducing manual tests that we always required to do. Otherwise we don't have a way of catching bugs inside wrappers, the place where commands pkg does IO which is very important.

I too prefer using the native testing utilities that can be provided by the pkg itself. But testing how coreapi is used in commands pkg is easier by mocking the client side funcs of coreapi as I experimented. We had the same problem before with the go-application and go-service. We didn't use mocks there and implemented a fake net.Conn, which is a bit harder to maintain when compared to mocking because starting a fake gRPC server causes some async behavior that needed be synced otherwise it introduces bug into tests.

For daemon, we already have its interface so the best option is is to create mocks on top of that.

[krhubert]

Survey pkg doesn't provide a nice way of testing as I already checked so, it's a proper option to use mocks here.

This is the point that it isn't good to use mocks. Again Survey pkg option for testing is still better then modifying code in order to introduce the Survey interface. Maybe there is an option to write Survery bridge. I don't know.

I don't mind adopting a better way. Could you please provide one?

I don't mind either, but as I said there is have no one at this moment, but shouldn't be the root cause of introducing mocks, because maybe there is another way that needs to be discovered.

For the rest of things is a different discussion, but in general, having too many tests as bad as having too little. And about the other things we can talk in different thread/PR