Generator implementation for V2 smoke tests for C2J files. #3578

AlexDaines · 2024-12-12T18:11:34Z

Description

Implements the generator for smoke tests v2 format based on the new smoke-2.json schema. Key features:

Supports all configuration options from the smoke-2.json schema
Handles complex input types including arrays and nested objects
Generates both success and failure test cases
Includes proper test categorization
Manages special cases for Tags and Includes objects

Questions for review:

Special case handling for Tags/Includes - should this be more generic?
Configuration defaults - should we explicitly set schema defaults?
Error handling - any additional checks needed beyond ErrorCode?
Input validation - should we validate against operation parameters?
Test categorization - any additional AWS-specific categories needed?

Motivation and Context

This implementation supports the new smoke tests format (smoke-2.json) which provides more configuration options and better structure for test cases. The generator creates test files that:

Allow per-test configuration
Support all endpoint configuration options
Handle complex input structures
Provide proper error handling and validation

Testing

Verified generation with ACM service model
Tested various scenarios including:
- Success and failure cases
- Complex input structures (arrays, nested objects)
- Special cases (Tags, Includes)
- Edge cases (null values, empty strings)
- All configuration options
Generated tests compile successfully

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project
My change requires a change to the documentation
I have updated the documentation accordingly
I have read the README document
I have added tests to cover my changes
All new and existing tests passed

License

I confirm that this pull request can be released under the Apache 2 license

generator/ServiceClientGeneratorLib/Generators/TestFiles/SmokeTestsV2.tt

normj · 2024-12-13T07:52:35Z

generator/ServiceClientGeneratorLib/Generators/TestFiles/SmokeTestsV2.tt

+                }
+            }
+#>
+        public async Task <#=testCase["id"]?.ToString() ?? "Test"#>()


Were there tests without an id field? Seems like a field that would be required.

generator/ServiceClientGeneratorLib/Generators/TestFiles/SmokeTestsV2.tt

normj · 2024-12-13T08:02:01Z

generator/ServiceClientGeneratorLib/Generators/TestFiles/SmokeTestsV2.tt

+                            }
+                            else
+                            {
+                                var arrayValues = new List<string>();


Can't remember in the SEP can there be arrays of more then just strings.

dscpinheiro · 2024-12-17T23:44:59Z

generator/ServiceClientGeneratorLib/Generators/TestFiles/SmokeTestsV2.tt

@@ -8,6 +8,7 @@
 	AddLicenseHeader();
 #>
 using System;
+using System.Collections.Generic;
 using System.Threading.Tasks;
 using Microsoft.VisualStudio.TestTools.UnitTesting;


(I'm not reviewing the PR, but...) Is there a reason why you're using MSTest as the test framework?

The hand-written files we have right now use xUnit (meaning they can run in parallel) and using xUnit will allow you to test your PR without making any changes to our build system (as long as they're found by the projects in this folder: https://github.com/aws/aws-sdk-net/tree/v4-development/sdk/test/SmokeTests).

unless you want to make a buildsystem change I'd probably listen to daniel here

peterrsongg · 2025-01-08T01:25:35Z

the file isn't in this PR but shouldn't you remove this logic in GeneratorDriver.cs?

            if (this.Configuration.ServiceModel.HasSmokeTestsV2 && 
                (this.Configuration.ServiceId == "API Gateway" || this.Configuration.ServiceId == "ACM") )
            {
                ExecuteIntegrationTestGenerator(new SmokeTestsV2(), "SmokeTestsV2.cs");
            }

I'm assuming that was just put there for testing purposes, but we should be generating smoke tests for every service

peterrsongg · 2025-01-08T01:27:26Z

~~I tried running the generator and it seems like this needs to be changed as well~~

                ExecuteIntegrationTestGenerator(new SmokeTestsV2(), "SmokeTestsV2.cs");

~~with this logic, each file will be SmokeTestsV2.cs and one will overwrite the other. You should~~~~probably do something similar to the other generators and do something like~~

ExecuteIntegrationTestGenerator(new SmokeTestsV2(), serviceName + "SmokeTestsV2.cs");

~~Either that or something weird is going on with the generator, but I don't see the files in there for some reason~~

hmm so actually it is generating it, but then it is being removed and the output of the generator is showing

**** Warning: Removing orphaned generated test code SmokeTestsV2.cs

and that's why it wasn't showing up for me.

peterrsongg · 2025-01-08T02:02:01Z

^^ looking into the problem I mentioned above a bit more, it seems like it thinks these generated integ tests are orphaned files because they are not a part of the generatedTestFiles that get populated here
https://github.com/aws/aws-sdk-net/blob/main/generator/ServiceClientGenerator/Program.cs#L64-L79

since it is not part of that list, when it is passed into GeneratorDriver.RemoveOrphanedShapesAndServices it gets removed. This is probably the first instance of a generated integ tests so you should add these generated integ tests to the list so it isn't removed.

The removing of the orphaned code should definitely be fixed, since it makes it kind of hard to review without knowing what every smoke test file looks like.

peterrsongg · 2025-01-08T22:29:59Z

generator/ServiceClientGeneratorLib/Generators/TestFiles/SmokeTestsV2.tt

-            var response = await serviceClient.<#=modeledOperation.Name#>Async(request);
+<#
+            var input = testCase["input"] as JsonData;
+            if (input != null && input.IsObject)


is there a reason we need to check input.IsObject? if input != null, then we are guaranteed to be given a json object wityh the input so I think we can remove the IsObject check.

peterrsongg · 2025-01-08T22:33:46Z

generator/ServiceClientGeneratorLib/Generators/TestFiles/SmokeTestsV2.tt

+                foreach (string jsonKey in input.PropertyNames)
+                {
+                    var propertyName = FindPropertyName(modeledOperation, jsonKey);
+                    var value = input[jsonKey];


nit: since the name is propertyName, you could make it consistent and say propertyValue for the value. fine with me if u dont want to change though

peterrsongg · 2025-01-08T22:36:15Z

generator/ServiceClientGeneratorLib/Generators/TestFiles/SmokeTestsV2.tt

+<#
+                                foreach (JsonData tag in value)
+                                {
+                                    if (tag != null && tag.IsObject && tag["Key"] != null && tag["Value"] != null)


same comment here about IsObject. Is it required?

peterrsongg · 2025-01-08T22:58:19Z

generator/ServiceClientGeneratorLib/Generators/TestFiles/SmokeTestsV2.partial.cs

+            return (bool)useAccountIdRouting;
+        }
+
+        public string[] GetSigV4aRegionSet(JsonData testCase)


you could just make the return type List<string>, which would remove the extra regions.ToArray() call at the end and instead just return regions.

peterrsongg · 2025-01-08T23:00:49Z

generator/ServiceClientGeneratorLib/Generators/TestFiles/SmokeTestsV2.tt

+            if (IsSuccessExpected(testCase))
+            {
+#>
+            var response = await serviceClient.<#=modeledOperation.Name#>Async(request);


Add .ConfigureAwait(false) at the end of this async call. We should always be adding that at the end of async calls, unless you really know what you're doing. See https://devblogs.microsoft.com/dotnet/configureawait-faq/#why-would-i-want-to-use-configureawait(false) for why we should use configureAwait(false)

peterrsongg · 2025-01-08T23:02:40Z

generator/ServiceClientGeneratorLib/Generators/TestFiles/SmokeTestsV2.tt

+#>
+            try
+            {
+                var response = await serviceClient.<#=modeledOperation.Name#>Async(request);


same comment as above

peterrsongg · 2025-01-08T23:04:36Z

generator/ServiceClientGeneratorLib/Generators/TestFiles/SmokeTestsV2.tt

@@ -8,6 +8,7 @@
 	AddLicenseHeader();
 #>
 using System;
+using System.Collections.Generic;
 using System.Threading.Tasks;
 using Microsoft.VisualStudio.TestTools.UnitTesting;


unless you want to make a buildsystem change I'd probably listen to daniel here

Generator implementation for V2 smoke tests for C2J files.

e6cd047

AlexDaines requested a review from normj December 12, 2024 18:11

AlexDaines mentioned this pull request Dec 12, 2024

Add smoke tests v2 generator implementation #3577

Closed

10 tasks

eddiemcs3 requested review from boblodgett and gcbeattyAWS December 12, 2024 18:41

normj requested changes Dec 13, 2024

View reviewed changes

Bug fix for cases of null errorId, small changes to align with SEP.

dcef01b

dscpinheiro reviewed Dec 17, 2024

View reviewed changes

peterrsongg requested review from peterrsongg and removed request for gcbeattyAWS and boblodgett January 8, 2025 01:48

peterrsongg requested changes Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generator implementation for V2 smoke tests for C2J files. #3578

Generator implementation for V2 smoke tests for C2J files. #3578

AlexDaines commented Dec 12, 2024

normj Dec 13, 2024

normj Dec 13, 2024

dscpinheiro Dec 17, 2024

AlexDaines Jan 5, 2025

peterrsongg Jan 8, 2025

peterrsongg commented Jan 8, 2025

peterrsongg commented Jan 8, 2025 •

edited

Loading

peterrsongg commented Jan 8, 2025 •

edited

Loading

peterrsongg Jan 8, 2025

peterrsongg Jan 8, 2025

peterrsongg Jan 8, 2025

peterrsongg Jan 8, 2025

peterrsongg Jan 8, 2025

peterrsongg Jan 8, 2025

peterrsongg Jan 8, 2025

Generator implementation for V2 smoke tests for C2J files. #3578

Are you sure you want to change the base?

Generator implementation for V2 smoke tests for C2J files. #3578

Conversation

AlexDaines commented Dec 12, 2024

Description

Motivation and Context

Testing

Types of changes

Checklist

License

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

peterrsongg commented Jan 8, 2025

peterrsongg commented Jan 8, 2025 • edited Loading

peterrsongg commented Jan 8, 2025 • 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

peterrsongg commented Jan 8, 2025 •

edited

Loading

peterrsongg commented Jan 8, 2025 •

edited

Loading