Add unit tests to common package for allowedValues change #641

damntrecky · 2023-12-06T22:08:01Z

Problem

We rushed out a fix yesterday (#638) because there were allowedValues with spaces in the values and this wasn't supported by the build here. We didn't add unit tests for the fix to convert spaces to _ underscores

Solution

Unfortunately it looks like for Kotlin and C# the allowedValues enum, or output metdata key WAS converted, but the value itself was NOT converted to have _ underscores.

This PR fixes that and adds unit tests for all three languages. I also updated the README for each section as it might not be obvious for developers on how to run the tests for each language. Especially if they are not familiar with that language.

License

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

…ores

… kotlin

…scores

damntrecky · 2023-12-06T22:13:15Z

telemetry/csharp/AwsToolkit.Telemetry.Events.Generator/DefinitionsBuilder.cs

@@ -213,7 +213,7 @@ private CodeTypeDeclaration GenerateEnumStruct(MetricType type)
                    allowedValue.Replace(" ", "_").ToPascalCase().Replace(".", "").Replace("-", ""))
                {
                    InitExpression = new CodeObjectCreateExpression(type.GetGeneratedTypeName(),


For C# we converted the "key" but not the "value" to get rid of spaces. This was an oversight and testing of course caught this.

Thanks for pushing for testing.

Were any Toolkits released with spaces in the values? This change would create a discrepancy in emitted values across Toolkit versions.

Why not change the definition data to remove the spaces? I don't understand why the values cannot contain spaces.

There were spaces deployed this week. But we can revert this quickly as our slack discussion aligns with this being better for backend ingestion now and in the future.

telemetry/csharp/AwsToolkit.Telemetry.Events.Tests/GeneratedCodeTests.cs

damntrecky · 2023-12-06T22:13:52Z

telemetry/csharp/AwsToolkit.Telemetry.Events.Tests/GeneratedCodeTests.cs

+            Assert.True(datum.Metadata.ContainsKey("codeTransformSessionId"));
+            Assert.Equal("test-session-id", datum.Metadata["codeTransformSessionId"]);
+            Assert.True(datum.Metadata.ContainsKey("codeTransformPreValidationError"));
+            Assert.Equal("No_Java_project_found", datum.Metadata["codeTransformPreValidationError"]);


Strictly assert the new change/value here.

telemetry/jetbrains/src/test/resources/testGeneratorAllowedValuesOutput

damntrecky · 2023-12-06T22:14:42Z

...ry/jetbrains/src/main/kotlin/software/aws/toolkits/telemetry/generator/TelemetryGenerator.kt

@@ -86,7 +86,7 @@ private fun FileSpec.Builder.generateTelemetryEnumType(item: TelemetryMetricType
    item.allowedValues!!.forEach { enumValue ->
        enum.addEnumConstant(
            enumValue.toString().replace(Regex("\\s"), "_").toTypeFormat(), TypeSpec.anonymousClassBuilder()


For Kotlin we converted the "key" but not the "value" to get rid of spaces. This was an oversight and testing of course caught this.

Thanks for pushing for testing.

damntrecky · 2023-12-06T22:15:08Z

/retryBuilds

- revert value conversion from _ and just conver key - update definition for preValidation - fix unit tests

…olkit-common into nardeck/add-unit-tests

damntrecky · 2023-12-06T22:51:52Z

telemetry/jetbrains/src/test/resources/testGeneratorAllowedValuesOutput

+public enum class TestAllowedValues(
+    private val `value`: String,
+) {
+    TestSpacesAreReplaced("test spaces are replaced"),


Key is converted, but NOT the value. In general the advice was to avoid spaces for values.

I would expect this not be a breaking change, since no other allowedValues have spaces other than the values my team introduced this week. This means the enum keys will only change for our team.

damntrecky · 2023-12-06T22:52:02Z

telemetry/definitions/commonDefinitions.json

-                "Only Maven projects supported",
-                "Empty project",
-                "Non SSO login",
-                "Project running on backend"


Update the values to NOT use spaces.

telemetry/vscode/src/generate.ts

damntrecky · 2023-12-08T22:22:58Z

/retrybuilds

damntrecky · 2023-12-08T22:23:50Z

/retryBuilds

damntrecky · 2023-12-11T17:39:22Z

Kotlin build failing fixed here -> https://github.com/aws/aws-toolkit-common/pull/646/files

I believe this current change WILL be a breaking changes for anyone who pulls it in and should be separate from the change above. Ideally, once this change is deployed my team can fix the breaking changes (which are for Elastic gumby/ amazon q code transform) intelliJ and vscode instances.

awschristou · 2023-12-11T17:41:57Z

@damntrecky this PR contains two separate changes:

updating enum values in telemetry/definitions/commonDefinitions.json to preferred values (removing spaces from values)
fixing and validating generators to handle spaces in values

These need to be split into two separate PRs. If we need to investigate a regression, or revert a change, this PR as-is makes the repo harder to manage. The enum values update is needed sooner than the validation update. Please create a separate PR with just that change. Once that is merged in, this PR can be revisited in order to handle the testing (which is less urgent, as we won't have enum values containing spaces).

damntrecky · 2023-12-11T17:42:06Z

telemetry/csharp/AwsToolkit.Telemetry.Events.Tests/GeneratedCodeTests.cs

@@ -217,6 +217,41 @@ public void RecordMetricWithNewTransform()
            Assert.Single(datum.Metadata);
        }

+        /// <summary>


Test to make sure allowedValues with spaces converts the key properly and make sure the allowedValue -> value has spaces for now.

Although its recommended to NOT have spaces in the values, the build should not fail and have consistent behavior across environments

justinmk3 · 2023-12-11T20:49:44Z

telemetry/csharp/AwsToolkit.Telemetry.Events.Tests/GeneratedCodeTests.cs

@@ -217,6 +217,41 @@ public void RecordMetricWithNewTransform()
            Assert.Single(datum.Metadata);
        }

+        /// <summary>
+        /// RecordCodeTransformIsDoubleClickedToTriggerInvalidProject was chosen as a sample call that has a 
+        /// CodeTransformPreValidationError for allowedValues which have spaces in the values that 


CodeTransformPreValidationError values no longer have spaces, does the comment need to be updated?

Nick Ardecky added 12 commits December 5, 2023 09:44

Feature - add shared telemetry metrics for AmazonQ Transform

d36a467

Chore - remove type codeTransformConfigurationFilePath

e6197ca

Chore - update metric description definitions text

66ae21f

Chore - remove isVisibleToUser field

088e482

Chore - add newline character to file

a16a54d

Chore - update description for total runtime

c24e11a

Chore - fix build isssue to replace allowedValues spaces with undersc…

a43ecff

…ores

Chore - follow up with jetbrains underscore conversion

76c203a

Chore - add allowValues parsing spaces to underscores tests

e394cc2

Chore - update vscode readme for testing

ad973de

Chore - Add unit tests for converting allowedValues to underscores in…

4f5bc6f

… kotlin

Chore - add testing for modifying allowedValues with spacees to under…

e27cccc

…scores

damntrecky requested a review from a team as a code owner December 6, 2023 22:08

Merge branch 'main' into nardeck/add-unit-tests

5f7a447

damntrecky commented Dec 6, 2023

View reviewed changes

telemetry/csharp/AwsToolkit.Telemetry.Events.Tests/GeneratedCodeTests.cs Show resolved Hide resolved

damntrecky commented Dec 6, 2023

View reviewed changes

telemetry/jetbrains/src/test/resources/testGeneratorAllowedValuesOutput Show resolved Hide resolved

damntrecky commented Dec 6, 2023

View reviewed changes

Nick Ardecky added 2 commits December 6, 2023 14:49

Chore - updates

6c3cf26

- revert value conversion from _ and just conver key - update definition for preValidation - fix unit tests

Merge branch 'nardeck/add-unit-tests' of github.com:damntrecky/aws-to…

7e7edc2

…olkit-common into nardeck/add-unit-tests

damntrecky commented Dec 6, 2023

View reviewed changes

telemetry/vscode/src/generate.ts Show resolved Hide resolved

Chore - update contribution readme

1c83705

damntrecky commented Dec 11, 2023

View reviewed changes

justinmk3 mentioned this pull request Dec 11, 2023

Feature - Amazon Q Code Transform Update #648

Merged

justinmk3 reviewed Dec 11, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit tests to common package for allowedValues change #641

Add unit tests to common package for allowedValues change #641

damntrecky commented Dec 6, 2023 •

edited by awschristou

Loading

damntrecky Dec 6, 2023

awschristou Dec 6, 2023

damntrecky Dec 8, 2023

damntrecky Dec 6, 2023

damntrecky Dec 6, 2023

damntrecky commented Dec 6, 2023

damntrecky Dec 6, 2023

damntrecky Dec 8, 2023 •

edited

Loading

damntrecky Dec 6, 2023

damntrecky commented Dec 8, 2023

damntrecky commented Dec 8, 2023

damntrecky commented Dec 11, 2023

awschristou commented Dec 11, 2023

damntrecky Dec 11, 2023

justinmk3 Dec 11, 2023

Add unit tests to common package for allowedValues change #641

Are you sure you want to change the base?

Add unit tests to common package for allowedValues change #641

Conversation

damntrecky commented Dec 6, 2023 • edited by awschristou Loading

Problem

Solution

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

damntrecky commented Dec 6, 2023

Choose a reason for hiding this comment

damntrecky Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damntrecky commented Dec 8, 2023

damntrecky commented Dec 8, 2023

damntrecky commented Dec 11, 2023

awschristou commented Dec 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damntrecky commented Dec 6, 2023 •

edited by awschristou

Loading

damntrecky Dec 8, 2023 •

edited

Loading