Helper library for starting containers in e2e tests #306

jrcamp · 2020-06-09T21:15:11Z

This provides a helper around starting and cleaning up containers as part of
end to end testing. See the redis e2e test as an example of how it would be
used.

Contributes to open-telemetry/opentelemetry-collector#1027

linux-foundation-easycla · 2020-06-09T21:15:15Z

The committers are authorized under a signed CLA.

✅ Jay Camp (9e2e600b83efeafda1617547bb8b6114e6358350)

asuresh4 · 2020-06-09T22:08:46Z

internal/common/testing/container/container.go

+}
+
+// StartContainer starts a container with the given image and zero or more ContainerOptions.
+func (c *Containers) StartContainer(image string, opts ...ContainerOption) Container {


Might also be useful to be able to pass in path to a Dockerfile from which the image can be built.

Yep we'll need that eventually.

pmcollins · 2020-06-10T14:32:14Z

receiver/redisreceiver/factory.go

@@ -45,7 +46,7 @@ func (f *Factory) Type() configmodels.Type {

 // CreateDefaultConfig creates a default config.
 func (f *Factory) CreateDefaultConfig() configmodels.Receiver {
-	return &config{}
+	return &config{CollectionInterval: 10 * time.Second}


Thanks for fixing this. :-)

jchengsfx

Need to add - setup_remote_docker to the test job in the circleci config.

owais

Any reason to place it under common? It could simple be internal/testing/container.

owais · 2020-06-10T14:46:10Z

internal/common/testing/container/container.go

+}
+
+// StartContainer starts a container with the given image and zero or more ContainerOptions.
+func (c *Containers) StartContainer(image string, opts ...ContainerOption) Container {


Should this be just Start() so API reads container.Start()? Is there anything else in this package that can be "started"?

Hmm we may have other ways to start (like @asuresh4 mentioned building from a Dockerfile and starting it which is something we do in Smart Agent). So there may be more than one kind of start. Maybe this could be StartImage and StartDockerfile in future?

Yea that sounds good or just Start(WithImage()) Start(WithDockerfile()).

owais · 2020-06-10T14:49:15Z

receiver/redisreceiver/redis_e2e_test.go

+
+func TestIntegration(t *testing.T) {
+	d := container.New(t)
+	c := d.StartContainer("docker.io/library/redis:6.0.3", container.WithPortReady(6379))


How are containers stopped? Should a test be responsible for stopping a container it starts?

They get stopped automatically by test framework (calls Cleanup). See how it registers itself with t.Cleanup in New().

owais · 2020-06-10T14:50:05Z

receiver/redisreceiver/redis_e2e_test.go

+
+func TestIntegration(t *testing.T) {
+	d := container.New(t)
+	c := d.StartContainer("docker.io/library/redis:6.0.3", container.WithPortReady(6379))


Would be nice to have a randomPort() int function that returns a randomly available port but not strictly required for this PR.

I think we have that function already somewhere. But yeah it's not needed here.

Could multiple tests run in parallel in same or different suites that both want to run redis?

They could but it'd still be fine. They'd get assigned different host ports at random.

keitwb · 2020-06-10T15:13:18Z

internal/common/testing/container/container.go

+	ports        nat.PortMap
+}
+
+func (c *Container) AddrForPort(port uint16) string {


Stay away from the host port bindings as much as possible with this, as long as you are using bridged networking with Docker, you can access the container IP directly from the test process and access any port you want without having to bind it with Docker. The host mappings just create more indirection and potential for port conflicts and have no real benefit.

Stay away from the host port bindings as much as possible with this, as long as you are using bridged networking with Docker, you can access the container IP directly from the test process and access any port you want without having to bind it with Docker.

Best I could tell after trying and reading the docs this doesn't work on Mac does it? I'm basing that on https://docs.docker.com/docker-for-mac/networking/#per-container-ip-addressing-is-not-possible.

The host mappings just create more indirection and potential for port conflicts and have no real benefit.

The host ports are assigned randomly which is why there's an AddrForPort function that looks up the container port to the mapped host port.

pmcollins · 2020-06-10T15:17:03Z

Very cool functionality overall. Am I correct in assuming that the unit test defined here won't be how these kinds of tests (that require Docker to be running) will be run?

keitwb · 2020-06-10T15:19:24Z

internal/common/testing/container/container.go

+		c.t.Fatalf("failed inspecting container %v: %v", created.ID, err)
+	}
+
+	if con.NetworkSettings.IPAddress == "" {


Access the container with this IP address instead of host port bindings. If you do that, you don't need to look at con.NetworkSettings.Ports at all.

I think I addressed this here: #306 (comment)

This approach seems to work on both Mac and CircleCI and haven't found any downsides to it (yet).

jrcamp · 2020-06-10T15:19:55Z

Very cool functionality overall. Am I correct in assuming that the unit test defined here won't be how these kinds of tests (that require Docker to be running) will be run?

Yeah I think for an end to end test it needs to be constructing things at a higher level to get more coverage so I'm going to experiment with that. (Maybe generating a yaml config file dynamically?) Or did you mean something else?

keitwb · 2020-06-10T15:21:32Z

internal/common/testing/container/container.go

+	for _, port := range container.waitForPorts {
+		ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+
+		func() {


This is a pretty hacky way to make use of defer.

pmcollins · 2020-06-10T15:27:03Z

Very cool functionality overall. Am I correct in assuming that the unit test defined here won't be how these kinds of tests (that require Docker to be running) will be run?

Yeah I think for an end to end test it needs to be constructing things at a higher level to get more coverage so I'm going to experiment with that. (Maybe generating a yaml config file dynamically?) Or did you mean something else?

I was just thinking that any tests that run when you say go test shouldn't require a service to be running in the background, such as Docker. So I was wondering if you were in agreement with that, and if so, what mechanism were you planning on for running these tests.

jrcamp · 2020-06-10T15:33:10Z

Very cool functionality overall. Am I correct in assuming that the unit test defined here won't be how these kinds of tests (that require Docker to be running) will be run?

Yeah I think for an end to end test it needs to be constructing things at a higher level to get more coverage so I'm going to experiment with that. (Maybe generating a yaml config file dynamically?) Or did you mean something else?

I was just thinking that any tests that run when you say go test shouldn't require a service to be running in the background, such as Docker. So I was wondering if you were in agreement with that, and if so, what mechanism were you planning on for running these tests.

Oh I see. Yeah when you run go test you will just get unit tests. To run these tests you will have to do something like go test -tags=integration ./...

jrcamp · 2020-06-10T19:01:23Z

Any reason to place it under common? It could simple be internal/testing/container.

Would like to but internal doesn't have a go.mod. Not sure if it can/should be reorganized or not.

jrcamp · 2020-06-11T03:52:09Z

Marking ready to review just so that coverage bot will run (assuming that's why it's not..) but it's still WIP.

codecov · 2020-06-11T04:01:17Z

Codecov Report

Merging #306 into master will increase coverage by 0.03%.
The diff coverage is 74.11%.

@@            Coverage Diff             @@
##           master     #306      +/-   ##
==========================================
+ Coverage   82.78%   82.82%   +0.03%     
==========================================
  Files         170      171       +1     
  Lines        9070     9147      +77     
==========================================
+ Hits         7509     7576      +67     
+ Misses       1236     1235       -1     
- Partials      325      336      +11

Flag	Coverage Δ
#integration	`63.40% <74.11%> (+63.40%)`	⬆️
#unit	`82.59% <0.00%> (-0.19%)`	⬇️

Impacted Files	Coverage Δ
internal/common/testing/container/container.go	`73.80% <73.80%> (ø)`
receiver/redisreceiver/factory.go	`40.00% <100.00%> (+40.00%)`	⬆️
receiver/kubeletstatsreceiver/factory.go	`64.10% <0.00%> (-35.90%)`	⬇️
receiver/kubeletstatsreceiver/kubelet/client.go	`92.85% <0.00%> (-2.85%)`	⬇️
receiver/redisreceiver/receiver.go	`89.47% <0.00%> (+89.47%)`	⬆️
receiver/redisreceiver/client.go	`100.00% <0.00%> (+100.00%)`	⬆️

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 a00790d...6597bc2. Read the comment docs.

This provides a helper around starting and cleaning up containers as part of end to end testing. See the redis e2e test as an example of how it would be used. The test library itself Doesn't have tests yet, mainly proof of concept and to get feedback. open-telemetry/opentelemetry-collector#1027

jrcamp · 2020-06-18T17:17:01Z

This is ready for review now. The coverage is a bit low due to a lot of error paths that would require mocking a lot of Docker API calls which is probably not worth doing.

asuresh4

LGTM. Just a couple of nits. Also, looks like e2e.yaml is empty and does not seem to be used anywhere. Is it intentional?

asuresh4 · 2020-06-22T17:08:41Z

internal/common/testing/container/container_integration_test.go

+
+	con.Cleanup()
+
+	assert.Nil(t, con.runningContainers, "started containers should be empty after cleanup")


Would you like to use the corresponding method from the require module instead? So we don't need to import both require and assert modules?

require and assert have different behaviors. Usually you use require if you want the test to fail immediately whereas asserts continue execution of the test so that additional assertions can run.

asuresh4 · 2020-06-22T17:09:24Z

receiver/redisreceiver/redis_e2e_test.go

+		t: t,
+	}))
+
+	assert.Eventuallyf(t, func() bool {


Same thing about using methods from require.

asuresh4 · 2020-06-22T17:20:31Z

receiver/redisreceiver/redis_e2e_test.go

+	}))
+
+	assert.Eventuallyf(t, func() bool {
+		return len(consumer.AllMetrics()) > 0


Would be nice to actually look for metrics by name (like in the Smart Agent tests). But I guess we can get to it once we have metric metadata.

jrcamp · 2020-06-22T20:22:52Z

LGTM. Just a couple of nits. Also, looks like e2e.yaml is empty and does not seem to be used anywhere. Is it intentional?

Yep not needed, removed.

jrcamp · 2020-06-23T23:12:00Z

@tigrannajaryan ready for merge. coverage is lower due to error conditions being hard to test without mocking docker library

This provides a helper around starting and cleaning up containers as part of end to end testing. See the redis e2e test as an example of how it would be used. The test library itself Doesn't have tests yet, mainly proof of concept and to get feedback. Contributes to open-telemetry/opentelemetry-collector#1027

Change all occurrences of value to pointer receivers Add meta sys files to .gitignore Code cleanup e.g. - Don't capitalize error statements - Fix ignored errors - Fix ambiguous variable naming - Remove unnecessary type casting - Use named params Fix #306

The InMemorySpanExporter provides a friendly interface to retrieving span information, reducing the need for mocking in unit tests. Signed-off-by: Alex Boten <[email protected]>

jrcamp requested review from pmcollins, asuresh4 and a team June 9, 2020 21:15

jrcamp marked this pull request as draft June 9, 2020 21:21

asuresh4 reviewed Jun 9, 2020

View reviewed changes

pmcollins reviewed Jun 10, 2020

View reviewed changes

jchengsfx suggested changes Jun 10, 2020

View reviewed changes

owais reviewed Jun 10, 2020

View reviewed changes

keitwb reviewed Jun 10, 2020

View reviewed changes

jrcamp marked this pull request as ready for review June 11, 2020 03:51

jrcamp added the WIP label Jun 11, 2020

jrcamp mentioned this pull request Jun 12, 2020

Tooling for integration tests #321

Merged

jrcamp marked this pull request as draft June 12, 2020 22:58

jrcamp marked this pull request as ready for review June 18, 2020 15:59

jrcamp removed the WIP label Jun 18, 2020

jrcamp assigned jrcamp and asuresh4 Jun 18, 2020

asuresh4 reviewed Jun 22, 2020

View reviewed changes

jrcamp added 2 commits June 22, 2020 16:27

Merge remote-tracking branch 'upstream/master' into redis-e2e

16ac5e5

remove unused file

34c532e

asuresh4 approved these changes Jun 23, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into redis-e2e

6597bc2

tigrannajaryan approved these changes Jun 24, 2020

View reviewed changes

tigrannajaryan merged commit 9bc1ce9 into open-telemetry:master Jun 24, 2020

mxiamxia referenced this pull request in mxiamxia/opentelemetry-collector-contrib Jul 22, 2020

Clean up deprecated attributes processors (#306)

2031c4b

rmfitzpatrick mentioned this pull request Sep 8, 2020

Adopt dynamic docker stats functionality #958

Merged

codeboten pushed a commit that referenced this pull request Nov 23, 2022

update references to main (#306)

f022385


		con.Cleanup()

		assert.Nil(t, con.runningContainers, "started containers should be empty after cleanup")

Helper library for starting containers in e2e tests #306

Helper library for starting containers in e2e tests #306

Conversation

jrcamp commented Jun 9, 2020 • edited by tigrannajaryan Loading

linux-foundation-easycla bot commented Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchengsfx left a comment

Choose a reason for hiding this comment

owais left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmcollins commented Jun 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrcamp commented Jun 10, 2020

Choose a reason for hiding this comment

pmcollins commented Jun 10, 2020

jrcamp commented Jun 10, 2020

jrcamp commented Jun 10, 2020

jrcamp commented Jun 11, 2020

codecov bot commented Jun 11, 2020 • edited Loading

Codecov Report

jrcamp commented Jun 18, 2020

asuresh4 left a 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

jrcamp commented Jun 22, 2020

jrcamp commented Jun 23, 2020

jrcamp commented Jun 9, 2020 •

edited by tigrannajaryan

Loading

linux-foundation-easycla bot commented Jun 9, 2020 •

edited

Loading

codecov bot commented Jun 11, 2020 •

edited

Loading