Refactored S3 test cases to remove repetition and use table drive tests #110

saurabh-hirani · 2020-04-18T13:34:50Z

The current code for s3_test.go has the following issues:

Test cases which should be subtests are added as individual tests.
Testing var t is passed throughout the code and should be consolidated to be used only with tests.
There is no support for table drive tests to make it easier to add more test cases.
While we are testing only list and nuke operations for buckets, it is not apparent from the test cases.

Refactored the code to fix the above issues. This has no impact on the current functionality as only the s3_test.go file has changed. This activity is also to ensure that adding cases for tagged and named deletions in the future is easier.

* Remove passing testing var t all across the code * Reduce repetitive main tests and add subtests * Subtests simplified in 2 categories - TestList and TestNuke * Have subtests generated via returned test functions to avail table driven tests

saurabh-hirani · 2020-04-27T10:27:10Z

@yorinasub17 @brikis98 - can you please check and let me know if this looks okay? Thanks.

brikis98

Thanks for the PR!

GitHub is not doing a great job of rendering the diff, so this was quite hard to read and compare the old to the new. Would appreciate a second set of eyes @yorinasub17.

brikis98 · 2020-04-30T07:48:40Z

aws/s3_test.go

+// genTestsBucketName generates a test bucket name.
+func genTestBucketName() string {
+	// Call UniqueID twice because even if the nth test tries to reuse a name of the first test
+	// AWS S3 deletion operation might be in progress after nuking.


Not sure I understand this explanation?

Wanted to have a little bit more randomness but yes the comment isn't clear and not needed. Removed.

brikis98 · 2020-05-03T11:07:09Z

aws/s3_test.go

-	objectCount       int
-	objectBatchsize   int
+// setAWSParams sets up common operations for nuke S3 tests.
+func setAWSParams() (AWSParams, error) {


NIT: perhaps newAWSParams?

brikis98 · 2020-05-03T11:08:54Z

aws/s3_test.go

+		// Verify that - before creating bucket - it should not exist
+		//
+		// Please note that we are not reusing awsParams.awsSession and creating a random session in a region other
+		// than the one in which the bucket is created - this is useful to test the sceanrio where the user has


Suggested change

// than the one in which the bucket is created - this is useful to test the sceanrio where the user has

// than the one in which the bucket is created - this is useful to test the scenario where the user has

brikis98 · 2020-05-03T11:09:27Z

aws/s3_test.go

+			if err == nil {
+				assert.Fail(t, "Did not fail for invalid batch size")
+			}
+			t.Logf("SUCCESS: Did not list buckets due to invalid batch size - %s", bucket.name)


We should not be using t.Logf for logging.

brikis98 · 2020-05-03T11:11:31Z

aws/s3_test.go

+	type testCaseStruct struct {
+		name string
+		args TestListS3BucketArgs
 	}
-}

-func TestList_EmptyS3Bucket_NoTags(t *testing.T) {
-	testListS3BucketsWrapper(t, []map[string]string{}, 10, true)
-}
+	var allTestCases []testCaseStruct

-func TestList_EmptyS3Bucket_WithoutFilterTag(t *testing.T) {
-	testListS3BucketsWrapper(t, []map[string]string{
+	testCases := []testCaseStruct{


NIT: The Go idiom typically used for table-driven tests declares the struct and the tests all in one:

var testCases = []struct { param1 string param2 string }{ {"aaa", "bbb"}, {"ccc", "ddd"}, }

Done for TestListS3Bucket . For TestNukeS3Bucket as per -

cloud-nuke/aws/s3_test.go

Line 428 in 7a99393

for _, bucketType := range []string{"NoVersioning", "Versioning"} {

- as we need to generate tests both for versioned and non versioned buckets - we have to create list of tests first and then execute them.

brikis98 · 2020-05-03T11:12:44Z

aws/s3_test.go

+
+	targetRegions := []string{awsParams.region}
+
+	return func(t *testing.T) {


t.Parallel()?

aws/s3_test.go

* Not using t.Logf * Fixing typos * Using t.Parallel in sub and main tests * Using idomatic golang test struct where possible

saurabh-hirani · 2020-05-05T05:41:11Z

@brikis98 @yorinasub17 - updated as per feedback.

yorinasub17

The update makes sense, but I am not sure this is the best implementation for readability. I think my biggest challenge with this is the test function generator pattern.

I think it would be easier to follow and understand what is going on if there was instead a function to drive the test runNukeTest(t *testing.T, args *Args) that then contains the combination of the generator routine and subtest function. Then, usage would be:

		t.Run(tc.name, func(t *testing.T) { runNukeTest(t, tc.args) })

This removes one layer of indirection and should help make it much easier to follow what is going on. What do you think?

aws/s3_test.go

yorinasub17 · 2020-05-05T15:20:57Z

aws/s3_test.go

+		isVersioned := false
+		if bucketType == "Versioning" {
+			isVersioned = true
+		}


NIT:

isVersioned := bucketType == "Versioning"

* Remove one level of indirection by combining subtest and setup for S3 list and nuke tests

saurabh-hirani · 2020-05-06T12:37:11Z

This removes one layer of indirection and should help make it much easier to follow what is going on. What do you think?

Thanks - that would lead to cleaner code. Fixed it.

saurabh-hirani · 2020-05-06T12:38:00Z

@yorinasub17 - updated as per feedback.

saurabh-hirani · 2020-05-13T13:31:43Z

@yorinasub17 - just wanted to check if the above looks good - I have updated the code as per your feedback. Thanks.

yorinasub17 · 2020-05-23T06:05:17Z

Hi @saurabh-hirani apologies for the delay on this. I've been super booked this month and haven't had the time to look at this. I am hoping to be able to review this again next week.

yorinasub17

Apologies for the long review cycle this time. I had to put this off multiple times, but finally had the time to look. This is very close, but had a few style nits.

I am committed to moving this across the finish line in the next two weeks. Let me know if you are not able to handle the updates, and I can try to find time to apply them myself.

yorinasub17 · 2020-05-26T14:46:13Z

aws/s3_test.go

+		Region: awsgo.String(region)},
+	)
+	return session, err
+}


NIT: since the namespace is shared across the entire package, this should probably be put in a different place or renamed to something specific to s3 tests.

Done - added S3Test prefix where possible for consistency.

yorinasub17 · 2020-05-26T14:46:36Z

aws/s3_test.go

 	region     string
 	awsSession *session.Session
 	svc        *s3.S3
-	bucketName string
 }


NIT: rename to something that is specific to S3.

This has been removed so not applicable.

yorinasub17 · 2020-05-26T14:47:14Z

aws/s3_test.go

+// TestS3Bucket represents a test S3 bucket
+type TestS3Bucket struct {
+	name        string
+	tags        []map[string]string


NIT: consider using the structs from aws sdk for type safety.

Checked and saw that there is no need for TestS3Bucket struct - easier to have functions which take args and instead of using types and getting into potential conflicts with AWS. create and addObject have been replaced by S3TestCreateBucket and S3TestBucketAddObject

yorinasub17 · 2020-05-26T14:48:10Z

aws/s3_test.go

 	if err != nil {
-		assert.Fail(t, errors.WithStackTrace(err).Error())
+		assert.Failf(t, "Failed to setup AWS params", errors.WithStackTrace(err).Error())


Use require.Fail or require.NoError to cut the test short. Otherwise, execution will continue.

Thanks - wasn't aware of that - have used require.NoError - removes the if/else check also.

yorinasub17 · 2020-05-26T14:50:52Z

aws/s3_test.go

+	allTestCases = append(allTestCases, testCaseStruct{
+		"EmptyBucket",
+		TestNukeS3BucketArgs{isVersioned: false, checkDeleteMarker: false, objectCount: 0, objectBatchsize: 0, shouldNuke: true},
+	})


NIT: statically define the list like you did in the other test instead of using append, for consistency.

Did that - had optimised in this case as EmptyBucket test does not need to deal with versioned/non-versioned flags but moved it in.

yorinasub17 · 2020-05-26T14:51:29Z

aws/s3_test.go

-	}, true)
-}
+	for _, bucketType := range []string{"NoVersioning", "Versioning"} {
+		isVersioned := bucketType == "Versioning"


NIT: use true and false directly in the range var.

Cannot do so as the tests need the Versioning v/s NonVersioning prefix

--- PASS: TestNukeS3Bucket/NoVersioning_EmptyBucket (9.39s) --- PASS: TestNukeS3Bucket/NoVersioning_BatchObjects_InvalidBatchSize_Over (9.97s) --- PASS: TestNukeS3Bucket/Versioning_EmptyBucket (9.60s) --- PASS: TestNukeS3Bucket/Versioning_AllObjects (24.06s)

Having true/false in range will lead to reinterpreting true false and setting Versioning/nonVersioning

You could do:

fmt.Sprintf("Versioning_%v_EmptyBucket", isVersioning)

But it's not a big deal so can keep this as is.

aws/s3_test.go

saurabh-hirani · 2020-06-01T05:05:40Z

Apologies for the long review cycle this time. I had to put this off multiple times, but finally had the time to look. This is very close, but had a few style nits.

I am committed to moving this across the finish line in the next two weeks. Let me know if you are not able to handle the updates, and I can try to find time to apply them myself.

Thanks @yorinasub17 - will check out the changes today and reply back in the next 1-2 days.

…code

* Add S3Test prefix to avoid conflicts with other files in this namespace + AWS core libs * Use require.NoError * Remove TestS3Bucket type and use direct functions

saurabh-hirani · 2020-06-01T16:59:13Z

@yorinasub17 updated as per feedback and added comments. Please check and let me know if this looks good.

saurabh-hirani · 2020-06-11T03:35:59Z

@yorinasub17 - can you please review the above changes and provide feedback if any more changes are required?

yorinasub17 · 2020-06-11T03:47:23Z

Apologies for the delay. I had been snoozing this and lost track of it amongst all the other things on my plate. I just took a pass and the updates look reasonable, so kicked off a build.

If that passes, we can go ahead and merge this in. Thanks!

yorinasub17 · 2020-06-11T04:14:12Z

Ok build passed! Will merge this in. Thanks for your contribution!

saurabh-hirani · 2020-06-11T04:23:15Z

Thanks @yorinasub17 and @brikis98 for the feedback!

Refactored the S3 test cases to:

fc850d8

* Remove passing testing var t all across the code * Reduce repetitive main tests and add subtests * Subtests simplified in 2 categories - TestList and TestNuke * Have subtests generated via returned test functions to avail table driven tests

saurabh-hirani requested review from brikis98 and yorinasub17 as code owners April 18, 2020 13:34

brikis98 reviewed May 3, 2020

View reviewed changes

Updating as per feedback:

7a99393

* Not using t.Logf * Fixing typos * Using t.Parallel in sub and main tests * Using idomatic golang test struct where possible

saurabh-hirani force-pushed the refactor-s3-test-code branch from 0906e49 to 7a99393 Compare May 5, 2020 05:38

yorinasub17 reviewed May 5, 2020

View reviewed changes

Updating as per feedback:

373eac4

* Remove one level of indirection by combining subtest and setup for S3 list and nuke tests

yorinasub17 reviewed May 26, 2020

View reviewed changes

saurabh-hirani added 2 commits June 1, 2020 18:35

Merge remote-tracking branch 'upstream/master' into refactor-s3-test-…

27da8a8

…code

Updated as per feedback:

ad8f3ba

* Add S3Test prefix to avoid conflicts with other files in this namespace + AWS core libs * Use require.NoError * Remove TestS3Bucket type and use direct functions

saurabh-hirani mentioned this pull request Jun 7, 2020

Adding feature to ignore list and nuke errors #114

Closed

yorinasub17 merged commit 5f9bc56 into gruntwork-io:master Jun 11, 2020

	// than the one in which the bucket is created - this is useful to test the sceanrio where the user has
	// than the one in which the bucket is created - this is useful to test the scenario where the user has


		targetRegions := []string{awsParams.region}

		return func(t *testing.T) {

Refactored S3 test cases to remove repetition and use table drive tests #110

Refactored S3 test cases to remove repetition and use table drive tests #110

Conversation

saurabh-hirani commented Apr 18, 2020 • edited

saurabh-hirani commented Apr 27, 2020

brikis98 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

saurabh-hirani commented May 5, 2020

yorinasub17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saurabh-hirani commented May 6, 2020

saurabh-hirani commented May 6, 2020

saurabh-hirani commented May 13, 2020 • edited

yorinasub17 commented May 23, 2020

yorinasub17 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

Choose a reason for hiding this comment

saurabh-hirani commented Jun 1, 2020

saurabh-hirani commented Jun 1, 2020

saurabh-hirani commented Jun 11, 2020

yorinasub17 commented Jun 11, 2020

yorinasub17 commented Jun 11, 2020

saurabh-hirani commented Jun 11, 2020

saurabh-hirani commented Apr 18, 2020 •

edited

saurabh-hirani commented May 13, 2020 •

edited