Integ tests following the Fargate tutorial #765

efekarakus · 2019-04-23T03:52:08Z

Description of changes: Add integration tests that follow our Fargate tutorial (https://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-cli-tutorial-fargate.html)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

efekarakus · 2019-04-23T03:55:01Z

make integ-test passes locally.
Dryrun on CodeBuild also passes:

Building ecs-cli... 
./scripts/build_binary.sh ./bin/local 
Running integration tests... 
go test -tags integ -v ./ecs-cli/integ/... 
?       github.com/aws/amazon-ecs-cli/ecs-cli/integ [no test files] 
?       github.com/aws/amazon-ecs-cli/ecs-cli/integ/cfn [no test files] 
?       github.com/aws/amazon-ecs-cli/ecs-cli/integ/cmd [no test files] 
=== RUN   TestCreateClusterWithFargateService 
--- PASS: TestCreateClusterWithFargateService (365.44s) 
    configure.go:42: Created config ecs-cli-canary-dec75407-a609-48b5-bff5-e583edee05ee-fargate-config 
    up.go:60: Created cluster ecs-cli-canary-dec75407-a609-48b5-bff5-e583edee05ee-fargate-cluster in stack amazon-ecs-cli-setup-ecs-cli-canary-dec75407-a609-48b5-bff5-e583edee05ee-fargate-cluster 
    compose.go:72: Created service with name e2e-fargate-test-service 
    compose.go:85: Project e2e-fargate-test-service has 1 running containers 
    compose.go:116: Scaled the service e2e-fargate-test-service to 2 tasks 
    compose.go:85: Project e2e-fargate-test-service has 2 running containers 
    compose.go:147: Deleted service e2e-fargate-test-service 
    cfn.go:58: Stack amazon-ecs-cli-setup-ecs-cli-canary-dec75407-a609-48b5-bff5-e583edee05ee-fargate-cluster does not exist as expected 
    down.go:51: Deleted stack amazon-ecs-cli-setup-ecs-cli-canary-dec75407-a609-48b5-bff5-e583edee05ee-fargate-cluster 
PASS 
ok      github.com/aws/amazon-ecs-cli/ecs-cli/integ/e2e 365.441s 
?       github.com/aws/amazon-ecs-cli/ecs-cli/integ/stdout  [no test files]

petderek

Always good to see more integ tests around :)

petderek · 2019-04-23T04:14:05Z

ecs-cli/integ/cfn/cfn.go

+		assert.FailNowf(t, "unexpected CloudFormation error during DescribeStacks", "wanted no errors, got %v", err)
+	}
+	if resp.Stacks == nil {
+		assert.FailNow(t, "stacks should not be nil")


FFTI... but you can delete some lines if you do this:

import "github.com/stretchr/testify/require" require.NoErrorf(t, err, "message", var) // similar functions for NotNil, Equal, etc

Thanks for sharing this! It made the code so much neater ✨

PettitWesley · 2019-04-23T17:36:38Z

ecs-cli/integ/cfn/cfn.go

+		if aerr, ok := err.(awserr.Error); ok {
+			switch aerr.Code() {
+			case "ValidationError":
+				t.Logf("Stack %s does not exist as expected", stackName)


nit: when I first read this statement I didn't realize it was saying that the test was successful. How about:

Success: Stack %s does not exist

Or something like that

PettitWesley · 2019-04-23T17:40:20Z

ecs-cli/integ/stdout/stdout.go

-// HasAllSnippets returns true if stdout contains each snippet in wantedSnippets, false otherwise.
-func (b Stdout) HasAllSnippets(t *testing.T, wantedSnippets []string) bool {
+// TestHasAllSnippets returns true if stdout contains each snippet in wantedSnippets, false otherwise.
+func (b Stdout) TestHasAllSnippets(t *testing.T, wantedSnippets []string) bool {


nit: TestHasAllOutput?

renamed to TestHasAllSubstrings

PettitWesley · 2019-04-23T17:43:03Z

ecs-cli/integ/cmd/compose.go

+	}
+
+	// Then
+	stdout.Stdout(out).TestHasAllSnippets(t, []string{


Hmm. I kind of think that it would be better to call ecs:DescribeService to verify that the command succeeded, rather than checking the ECS CLI output.

I'm fine with just validating the command output, but I'd prefer if in addition we also made an API call to verify/check.

Yeah, so I also debated about this as well. I wrote the tests from the point of view of a user. How do they validate if compose service scale worked? they probably run compose service ps. That's why fargate_service_test is structured like:

// Increase the number of running tasks cmd.TestServiceScale(t, project, 2) cmd.TestServicePs(t, project, 2)

In scenarios where it didn't seem satisfying, like in TestUp I use the cfnClient to validate that the stack's name is following our naming template.

I couldn't think of some integration to test behind the scenes to validate that it's doing the right thing that PS doesn't do for us. I'm definitely open to suggestions here though :)

Okay, that makes sense to me. This satisfies my concern :)

PettitWesley

          |    |    |
         )_)  )_)  )_)
        )___))___))___)\
       )____)____)_____)\\
     _____|____|____|____\\\__

---------\ /---------
^^^^^ ^^^^^^^^^^^^^^^^^^^^^
^^^^ ^^^^ ^^^ ^^
^^^^ ^^^

allisaurus · 2019-04-24T19:07:55Z

ecs-cli/Gopkg.toml

 [[constraint]]
  name = "github.com/stretchr/testify"
-  version = "1.2.1"
+  version = "=1.2.1"


curious: are there issues with later minor versions of testify which require us pinning to 1.2.1 specifically?

I pinned it down to a specific version number that we know that works, instead of the default that will use a higher patch version if it exists (see behavior in https://github.com/golang/dep/blob/master/docs/Gopkg.toml.md#version).
We previously ran into an issue before where running dep ensure -update on the aws-sdk-go resulted in a broken build 😝. By pinning it we know what we get.

Upgraded to "=1.3.0"

Ah ha! Didn't realize that's what broke us on the aws-sdk-go build before.

allisaurus · 2019-04-24T19:12:26Z

ecs-cli/integ/cmd/configure.go

+	"testing"
+
+	"github.com/stretchr/testify/require"
+


nit: extra whitespace

This is what goimports formats it as for me :/

Kept it as is.

¯_(ツ)_/¯

allisaurus · 2019-04-24T20:20:41Z

ecs-cli/integ/cmd/compose.go

+		p.Name,
+		"service",
+		"up",
+		"--create-log-groups",


Can you confirm the cmd behavior when the log groups already exist? My understanding is they'll be created the first time this is run, but creation is silently (or with a warning) skipped once they exist.

It doesn't look like we're asserting on the existence or name of the log groups later in the test, so I'm wondering how useful this is to include (here and in the docker-compose.yml)?

You're right it skips it with a WARNING:

INFO[0000] Using ECS task definition TaskDefinition="efe-test:1" WARN[0001] Failed to create log group tutorial in us-east-1: The specified log group already exists INFO[0001] Created an ECS service service=efe-test taskDefinition="efe-test:1" INFO[0002] Updated ECS service successfully desiredCount=1 force-deployment=false service=efe-test

Removed.

allisaurus · 2019-04-24T20:37:25Z

ecs-cli/integ/e2e/fargate_service_test.go

+        awslogs-group: tutorial
+        awslogs-region: us-east-1
+        awslogs-stream-prefix: wordpress`
+	err := ioutil.WriteFile("./docker-compose.yml", []byte(content), os.ModePerm)


I don't see a where this file (or ecs-params.yml) are cleaned up. What if someone was running the tests on their laptop vs. in CodeBuild (which would terminate the instance after exiting)? I'd rather these not create resources that stick around after the test finishes.

Instead of creating a "real" file in the current directory, can we create a (temp) file in a temp directory and pass the name of that file into compose with -f ? If we end up running multiple tests that invoke compose, using+overwriting the same file could lead to weird failures if something get missed. It also means we can't run the e2e tests in parallel one day (assuming they'll live in the same dir).

👍 great suggestion. Done! The ioutil.TempFile function is pretty great.

efekarakus

Logs from the latest CodeBuild run

go test -tags integ -v ./ecs-cli/integ/e2e/... 
=== RUN   TestCreateClusterWithFargateService 
--- PASS: TestCreateClusterWithFargateService (335.10s) 
    configure.go:42: Created config ecs-cli-canary-ead681d3-6257-4bcf-ab2b-84c11b41279e-fargate-config 
    up.go:58: Created cluster ecs-cli-canary-ead681d3-6257-4bcf-ab2b-84c11b41279e-fargate-cluster in stack amazon-ecs-cli-setup-ecs-cli-canary-ead681d3-6257-4bcf-ab2b-84c11b41279e-fargate-cluster 
    fargate_service_test.go:82: Created /tmp/docker-compose-543637569.yml successfully 
    fargate_service_test.go:113: Created /tmp/ecs-params-103284908.yml successfully 
    compose.go:74: Created service with name e2e-fargate-test-service 
    compose.go:85: Project e2e-fargate-test-service has 1 running containers 
    compose.go:118: Scaled the service e2e-fargate-test-service to 2 tasks 
    compose.go:85: Project e2e-fargate-test-service has 2 running containers 
    compose.go:151: Deleted service e2e-fargate-test-service 
    cfn.go:50: Success: stack amazon-ecs-cli-setup-ecs-cli-canary-ead681d3-6257-4bcf-ab2b-84c11b41279e-fargate-cluster does not exist 
    down.go:49: Deleted stack amazon-ecs-cli-setup-ecs-cli-canary-ead681d3-6257-4bcf-ab2b-84c11b41279e-fargate-cluster 
PASS 
ok      github.com/aws/amazon-ecs-cli/ecs-cli/integ/e2e 335.108s

efekarakus · 2019-04-24T22:01:55Z

ecs-cli/Gopkg.toml

 [[constraint]]
  name = "github.com/stretchr/testify"
-  version = "1.2.1"
+  version = "=1.2.1"


I pinned it down to a specific version number that we know that works, instead of the default that will use a higher patch version if it exists (see behavior in https://github.com/golang/dep/blob/master/docs/Gopkg.toml.md#version).
We previously ran into an issue before where running dep ensure -update on the aws-sdk-go resulted in a broken build 😝. By pinning it we know what we get.

Upgraded to "=1.3.0"

efekarakus · 2019-04-24T22:10:16Z

ecs-cli/integ/cmd/compose.go

+		p.Name,
+		"service",
+		"up",
+		"--create-log-groups",


You're right it skips it with a WARNING:

INFO[0000] Using ECS task definition TaskDefinition="efe-test:1" WARN[0001] Failed to create log group tutorial in us-east-1: The specified log group already exists INFO[0001] Created an ECS service service=efe-test taskDefinition="efe-test:1" INFO[0002] Updated ECS service successfully desiredCount=1 force-deployment=false service=efe-test

Removed.

efekarakus · 2019-04-24T22:15:54Z

ecs-cli/integ/cmd/configure.go

+	"testing"
+
+	"github.com/stretchr/testify/require"
+


This is what goimports formats it as for me :/

Kept it as is.

efekarakus · 2019-04-24T22:45:45Z

ecs-cli/integ/e2e/fargate_service_test.go

+        awslogs-group: tutorial
+        awslogs-region: us-east-1
+        awslogs-stream-prefix: wordpress`
+	err := ioutil.WriteFile("./docker-compose.yml", []byte(content), os.ModePerm)


👍 great suggestion. Done! The ioutil.TempFile function is pretty great.

efekarakus requested review from PettitWesley, SoManyHs, allisaurus and piradeepk April 23, 2019 03:52

petderek reviewed Apr 23, 2019

View reviewed changes

PettitWesley reviewed Apr 23, 2019

View reviewed changes

PettitWesley approved these changes Apr 24, 2019

View reviewed changes

allisaurus suggested changes Apr 24, 2019

View reviewed changes

efekarakus added 6 commits April 24, 2019 14:49

Remove cmd_ps test

602fe85

Remove cmd_up test

fb329a6

Add end-to-end test following the Fargate tutorial

04f96a1

Add logging to cmd tests

2d82f2f

Use require lib instead of assert

ebe9a86

Upgrade testify to 1.3.0 and create Compose file as a temp file

9f9dbe4

efekarakus commented Apr 24, 2019

View reviewed changes

efekarakus force-pushed the integ-tests/fargate-service branch from fae1d4d to 9f9dbe4 Compare April 24, 2019 23:01

allisaurus approved these changes Apr 24, 2019

View reviewed changes

efekarakus merged commit e92a91d into aws:dev Apr 24, 2019

efekarakus deleted the integ-tests/fargate-service branch April 24, 2019 23:19

Integ tests following the Fargate tutorial #765

Integ tests following the Fargate tutorial #765

Uh oh!

Conversation

efekarakus commented Apr 23, 2019

Uh oh!

efekarakus commented Apr 23, 2019

Uh oh!

petderek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PettitWesley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efekarakus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants