Feature/container: new Container type & unit tests #340

ilgooz · 2018-08-05T18:57:45Z

closes #283.
related with #337, #268.

It would be much simpler to review this PR by reviewing each commit individually.
Run tests with integration tag to enable integration tests: go test -tags=integration
new dockertest package provided within this PR is used while unit testing of container package and will be used by its consumer packages to mock Docker API.
Container type is introduced.
Minor improvements done on the container package.
All contexts replaced with WithTimeout contexts.

NicolasMahe · 2018-08-07T14:38:34Z

I will review first thing tomorrow morning ;)

@ilgooz If you want to start another dev from this branch before it's merged, you can start a new branch from your last commit ;)

NicolasMahe

Impressive PR 👍 one typo away from approval!

NicolasMahe · 2018-08-08T08:16:37Z

container/dockertest/client.go

+	serviceInspectWithRaw chan serviceInspectWithRawResponse
+	serviceRemove         chan serviceRemoveResponse
+	serviceLogs           chan serviceLogsResponse
+	contanerInspect       chan containerInspectResponse


typo on contanerInspect. Should be containerInspect.
This typo is also at 3 other places.

NicolasMahe · 2018-08-08T08:23:43Z

container/client_test.go

-	err := createSwarmIfNeeded(client)
-	assert.Nil(t, err)
-}
-
 // TODO: this tests break other tests on my machine


Let's remove completely this file. I don't think we really need integration test on the createSwarm functionality.
@antho1404 what do you think?

Why would you want to remove it ? if the test is ok let's keep it, after I agree swarm tests are not absolutely necessary but I think it's still really good to keep these tests except if there is a good reason to remove them

They are already commented out....
Let's remove the pollution.

https://github.com/mesg-foundation/core/blob/e0e344e6449caabd857f0eebaa7eb81e99236309/container/client_test.go#L3-L25

my bad you're right

I'm removing core/container/client_test.go file. @antho1404 are you ok with this?

i think we should refactor our integration tests too #351. So maybe we should keep this one as a reminder to create an integration test from it.

NicolasMahe · 2018-08-08T08:26:01Z

container/container.go

 	}
-	containers, err := client.ContainerList(context.Background(), types.ContainerListOptions{
+	ctx, cancel = context.WithTimeout(context.Background(), c.callTimeout)
+	defer cancel()


is it ok to assign cancel multiple time and call it using defer?
Is defer will really execute the 2 cancel or the last cancel 2 times?

yes it is ok. please see: https://play.golang.org/p/UeNDuSXbZz7

antho1404 · 2018-08-08T12:30:39Z

cmd/service/init.go

+	if err != nil {
+		log.Fatal(err)
+	}
+	defaultContainer = c


I think this is to initialize a defaultContainer in the CLI but this file is the file for the command mesg-core service init so that might not make sense to put it here but maybe more in the utils where it's used

Also if it's only used one time in the service/utils#buildDockerImage why not initialize it in this function directly

We are going to remove these lines after service package refactored. *Container will be initialized once and stored in the *Service struct. I don't mind where to put this temporary code, want me to move to utils for now?

yes that would be good, I'm afraid to forget it here because it's not really related to this command

antho1404 · 2018-08-08T12:34:27Z

container/client_test.go

-	err := createSwarmIfNeeded(client)
-	assert.Nil(t, err)
-}
-
 // TODO: this tests break other tests on my machine


my bad you're right

antho1404 · 2018-08-08T12:40:32Z

container/dockertest/testing.go

+}
+
+// LastContainerList returns a channel that receives call arguments of last *Client.ContainerList call.
+func (t *Testing) LastContainerList() <-chan ContainerListRequest {


I don't like much the naming with Last but i'm definitely not against it

antho1404 · 2018-08-08T12:49:04Z

daemon/init.go

+	"github.com/mesg-foundation/core/container"
+)
+
+var defaultContainer *container.Container


Isn't it better to create an instance every time we need it instead of creating it an init function ?
It would be good to avoid init functions that save a state.
This container is totally stateless so we can re-create it all the time and it will not change anything except some memory/cpu optimization but that are negligible for these features

After daemon refactored, *Container will be initialized only once and stored in the *Daemon struct. Until the refactor we have to initialize a Container type and store it in the package level to be able to use it. I won't recommend -also don't like- initializing Container every time it's needed because it makes additional Docker API calls in the initialization time. Please see here.

ok let's keep it like that for now, and for the initialization of the container every time, you're right with the docker api calls even if we could add a singleton on that or some stuff like that but we will think about this kind of optimization later
So this is ok now :)

I directly changed the corresponding commit from the history as you liked. This way we'll not see an irrelevant code in the init.go file.

antho1404 · 2018-08-08T12:50:23Z

service/init.go

+var defaultContainer *container.Container
+
+func init() {
+	c, err := container.New()


same remark here, do we really need a state and an init function ?

Yes, until we refactor service. This is just a temporary solution.

* move integration tests to their own files called with _integration_test.go * add dockertest package to easily test methods that calls Docker API

* add more unit tests to container_test.go, build_test.go, network_test.go * extend dockertest API accordingly

* add unit tests to service_test,go, shared_network_test.go, task_test.go, wait_test.go * extend dockertest API accordingly * small fixes

* update container tests accordingly

NicolasMahe

👍 Good to merge 🌮

ilgooz added the in progress label Aug 5, 2018

ilgooz changed the title ~~Feature/container~~ Feature/container: new Container type & unit tests Aug 5, 2018

ilgooz added refactoring unit testing labels Aug 5, 2018

ilgooz added this to the v1.1.0 milestone Aug 5, 2018

ilgooz force-pushed the feature/container branch 6 times, most recently from 7a21585 to a6edac2 Compare August 7, 2018 09:32

NicolasMahe removed the in progress label Aug 7, 2018

ilgooz force-pushed the feature/container branch 3 times, most recently from 799e332 to 6a04217 Compare August 7, 2018 14:11

ilgooz requested review from antho1404, NicolasMahe and a team and removed request for antho1404 August 7, 2018 14:16

ilgooz force-pushed the feature/container branch from 6a04217 to e0e344e Compare August 7, 2018 14:18

ilgooz requested review from antho1404 and removed request for antho1404 August 7, 2018 14:19

ilgooz added the high priority label Aug 7, 2018

NicolasMahe requested changes Aug 8, 2018

View reviewed changes

antho1404 reviewed Aug 8, 2018

View reviewed changes

introduce Container type

d33013e

ilgooz added 3 commits August 8, 2018 19:27

container: improve tests & introduce dockertest package

ca06278

* move integration tests to their own files called with _integration_test.go * add dockertest package to easily test methods that calls Docker API

container: prepare for unit testing, add some unit tests to container.go

7d18549

add more unit tests for container package & extend dockertest API

c44a964

* add more unit tests to container_test.go, build_test.go, network_test.go * extend dockertest API accordingly

ilgooz force-pushed the feature/container branch from e0e344e to f29179a Compare August 8, 2018 16:27

ilgooz added 8 commits August 9, 2018 08:41

make non Newable packages compatible with container package

ce23fd0

add more unit tests for container package & extend dockertest API

97d8865

* add unit tests to service_test,go, shared_network_test.go, task_test.go, wait_test.go * extend dockertest API accordingly * small fixes

refactor container/dockertest

8b3d615

* update container tests accordingly

container: use contexts WithTimeout for Docker API calls

3c19b09

improve comments in container and container/dockertest packages

8e9b767

container: minor improvements on tests

8e2d277

add tests for container/dockertest, closes #341

3ed6589

fix typo on dockertest

c6c680d

ilgooz force-pushed the feature/container branch from f29179a to c6c680d Compare August 9, 2018 05:42

NicolasMahe removed this from the v1.1.0 milestone Aug 9, 2018

NicolasMahe approved these changes Aug 9, 2018

View reviewed changes

antho1404 approved these changes Aug 9, 2018

View reviewed changes

Merge branch 'dev' into feature/container

18a8675

antho1404 merged commit 8c48040 into dev Aug 9, 2018

antho1404 deleted the feature/container branch August 9, 2018 10:54

ilgooz mentioned this pull request Aug 13, 2018

add Internal Changes section to CHANGELOG #363

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/container: new Container type & unit tests #340

Feature/container: new Container type & unit tests #340

ilgooz commented Aug 5, 2018 •

edited

Loading

NicolasMahe commented Aug 7, 2018

NicolasMahe left a comment •

edited

Loading

NicolasMahe Aug 8, 2018

ilgooz Aug 8, 2018

NicolasMahe Aug 8, 2018

antho1404 Aug 8, 2018

NicolasMahe Aug 8, 2018 •

edited

Loading

NicolasMahe Aug 8, 2018

antho1404 Aug 8, 2018

ilgooz Aug 8, 2018 •

edited

Loading

ilgooz Aug 8, 2018

NicolasMahe Aug 8, 2018

ilgooz Aug 8, 2018 •

edited

Loading

antho1404 Aug 8, 2018

ilgooz Aug 8, 2018

antho1404 Aug 8, 2018

antho1404 Aug 8, 2018

antho1404 Aug 8, 2018

antho1404 Aug 8, 2018

ilgooz Aug 8, 2018

antho1404 Aug 8, 2018

ilgooz Aug 9, 2018

antho1404 Aug 8, 2018

ilgooz Aug 8, 2018

NicolasMahe left a comment •

edited

Loading

Feature/container: new Container type & unit tests #340

Feature/container: new Container type & unit tests #340

Conversation

ilgooz commented Aug 5, 2018 • edited Loading

NicolasMahe commented Aug 7, 2018

NicolasMahe 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

NicolasMahe Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilgooz Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilgooz Aug 8, 2018 • 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

NicolasMahe left a comment • edited Loading

Choose a reason for hiding this comment

ilgooz commented Aug 5, 2018 •

edited

Loading

NicolasMahe left a comment •

edited

Loading

NicolasMahe Aug 8, 2018 •

edited

Loading

ilgooz Aug 8, 2018 •

edited

Loading

ilgooz Aug 8, 2018 •

edited

Loading

NicolasMahe left a comment •

edited

Loading