-
Notifications
You must be signed in to change notification settings - Fork 862
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generator implementation for V2 smoke tests for C2J files. #3578
base: normj/v4-smoketests
Are you sure you want to change the base?
Conversation
} | ||
catch (Amazon<#=this.Config.ClassName#>Exception ex) | ||
{ | ||
Assert.AreEqual("<#=errorId#>", ex.ErrorCode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run this locally I get an exception because the <#=
doesn't handle errorId
being null. You need to add a null check before adding the Assert.AreEqual
.
} | ||
} | ||
#> | ||
public async Task <#=testCase["id"]?.ToString() ?? "Test"#>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were there tests without an id
field? Seems like a field that would be required.
if (value.IsString) | ||
{ | ||
#> | ||
request.<#=key#> = "<#=value.ToString()#>"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are going to be times that the key
defined in the smoke json file do not match exactly to the property name on the request. We change the case to match .NET conventions and sometimes have customizations to rename it. Look at how I wrote the FindOperation
method that looks in the ServiceModel
to find the shape by the model name versus the .NET name.
} | ||
else | ||
{ | ||
var arrayValues = new List<string>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't remember in the SEP can there be arrays of more then just strings.
@@ -8,6 +8,7 @@ | |||
AddLicenseHeader(); | |||
#> | |||
using System; | |||
using System.Collections.Generic; | |||
using System.Threading.Tasks; | |||
using Microsoft.VisualStudio.TestTools.UnitTesting; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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).
Description
Implements the generator for smoke tests v2 format based on the new smoke-2.json schema. Key features:
Questions for review:
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:
Testing
Types of changes
Checklist
License