fix(codeguruprofiler): incorrect profiling group name is returned when using importing

[openwebsolns]

Closes #22546. Verified via TDD with new unit test. Before the change, the unit test failed, replicating the bug in the issue with output:

FAIL test/profiling-group.test.js
  profiling group
    ✓ attach read permission to Profiling group via fromProfilingGroupArn (71 ms)
    ✓ attach publish permission to Profiling group via fromProfilingGroupName (27 ms)
    ✕ use name specified via fromProfilingGroupName (8 ms)
    ✓ default profiling group (21 ms)
    ✓ allows setting its ComputePlatform (14 ms)
    ✓ default profiling group without name (13 ms)
    ✓ default profiling group without name when name exceeding limit is generated (18 ms)
    ✓ grant publish permissions profiling group (25 ms)
    ✓ grant read permissions profiling group (25 ms)

  ● profiling group › use specified via name via fromProfilingGroupName

    expect(received).toEqual(expected) // deep equality

    Expected: "MyAwesomeProfilingGroup"
    Received: "profilingGroup"

      174 |
      175 |     const profilingGroup = ProfilingGroup.fromProfilingGroupName(stack, 'MyProfilingGroup', 'MyAwesomeProfilingGroup');
    > 176 |     expect(profilingGroup.profilingGroupName).toEqual('MyAwesomeProfilingGroup');
          |                                               ^
      177 |   });
      178 |
      179 |   test('default profiling group', () => {

      at Object.<anonymous> (test/profiling-group.test.ts:176:47)

---

All Submissions:

• [Yes] Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

• [No] This PR adds new unconventional dependencies following the process described here

New Features

• [N/A] Have you added the new feature to an integration test?
  • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

[gitpod-io]

[aws-cdk-automation]

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

[TheRealAmazonKendra]

All hail the almighty PR Linter. i.e., we need an integration test for this change.

[openwebsolns]

All hail the almighty PR Linter. i.e., we need an integration test for this change.

I did add a unit test. Are the integration tests separate?

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

[Naumel]

@Mergifyio update

[mergify]

update

✅ Branch has been successfully updated

[openwebsolns]

@TheRealAmazonKendra The workflow is saying changes requested by you, but it's not clear to me what else needs to happen. Thanks!

[TheRealAmazonKendra]

Instead of reusing an existing test, I'd prefer to see a new test with two test cases: one where you explicitly set the profilingGroupName and one where you don't explicitly set it. With the new style of integration tests (see INTEGRATION_TESTS), you can have multiple test cases and use assertions to test the value. In this case, it's just returning a reference to the Profiling group itself, not to the profilingGroupName. In CloudFormation, this may come out correct, but just reading the raw output with the reference makes it look incorrect as line 6 of the snapshot says

"ProfilingGroupName": "ProfilerGroupIntegrationTestMyProfilingGroup81DA69A3"

while the output of your test says

 "Outputs": {
  "MyProfilingGroupName": {
   "Value": {
    "Ref": "MyProfilingGroup829F0507"
   }
  }
 },

[openwebsolns]

See comment below.

[TheRealAmazonKendra]

The output is actually good if you do something like this:

const test = new ProfilerGroupIntegrationTest(app, 'ProfilerGroupIntegrationTest');
const testCase = new IntegTest(app, 'test', {
  testCases: [test],
});

const describe = testCase.assertions.awsApiCall('CloudFormation', 'describeStacks', {
  StackName: 'ProfilerGroupIntegrationTest',
});

describe.assertAtPath('Stacks.0.Outputs.0.OutputKey', ExpectedResult.stringLikeRegexp('MyProfilingGroupName'));
describe.assertAtPath('Stacks.0.Outputs.0.OutputValue', ExpectedResult.stringLikeRegexp('ProfilerGroupIntegrationTestMyProfilingGroup81DA69A3'));

This way you can test the actual values of the output. You'll just need to use our new IntegTest construct when writing the tests.

[TheRealAmazonKendra]

Given that it looks right here, I feel pretty confident about your change, I'd just like to see the correct raw output in the integ tests like here.

[openwebsolns]

I'm not sure how to trigger the raw output as you requested in the integration tests, which is why I just went with unit tests originally. The problem is forcing the resolution the name instead of the Ref token as you noted. I couldn't find any hints of this happening. Any code tips? I thought for this specific fix the unit test sufficed.

I was happy to be able to use test-driven development to both replicate the bug and assert its correctness. I found no value from the integration tests given that I have to accept the snapshot, and as you've pointed out it doesn't actually "cover" the fix.

Pull request has been modified.

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 4056598
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).