Skip to content
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

integ-tests: invalid AssertionResults logical ID #29700

Closed
nmussy opened this issue Apr 3, 2024 · 4 comments · Fixed by #29705
Closed

integ-tests: invalid AssertionResults logical ID #29700

nmussy opened this issue Apr 3, 2024 · 4 comments · Fixed by #29705
Labels
@aws-cdk/integ-tests bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@nmussy
Copy link
Contributor

nmussy commented Apr 3, 2024

Describe the bug

When adding an assertion to a httpApiCall, the CDK generates an invalid logical ID for its CfnOutput

Expected Behavior

The generated CfnOutput should have a valid logical ID

Current Behavior

The URL used to override the CfnOutput logical ID is not sluggified:

new CfnOutput(node, 'AssertionResults', {
value: result,
}).overrideLogicalId(`AssertionResults${id}`);

Reproduction Steps

Raised by Tobias Vollmer on https://cdk.dev/:

import { ExpectedResult, IntegTest } from '@aws-cdk/integ-tests-alpha';
import { App, Stack } from 'aws-cdk-lib';
const app = new App();
const testStack = new Stack(app, 'integTestStack', {});
const integ = new IntegTest(app, 'BaseIntegrationTest', {
  testCases: [testStack], regions: ['eu-central-1'] });

integ.assertions.httpApiCall(
  'https://httpbin.org/get',
  {}
).expect( 
  ExpectedResult.objectLike({
    status: 200,
  })
);
$ npx integ-runner --directory ./test-integ-tests --parallel-regions eu-central-1 --force
Deployment failed: Error [ValidationError]: Template format error: Outputs name 'AssertionResultsHttpApiCallhttpbin.org/get0f06632dfa7261b35a1569da58f981ba' is non alphanumeric.

I confirmed this by adding this unit test to deploy-assert.test.ts:

test.only('expect creates a valid CfnOutput', () => {
  // GIVEN
  const app = new App();
  const deplossert = new DeployAssert(app);

  // WHEN
  const query = deplossert.httpApiCall('https://example.com/test/123');
  query.expect(ExpectedResult.objectLike({ status: 200 }));

  // THEN
  Template.fromStack(deplossert.scope).hasOutput(
    'AssertionResultsHttpApiCallexample.com/test/1235ffa3a1b41e83da401e71706d1d9bc9a',
    {
      Value: {
        'Fn::GetAtt': ['HttpApiCallexamplecomtest1235ffa3a1b41e83da401e71706d1d9bc9a', 'assertion'],
      },
    },
  );
});

Possible Solution

Slugify the URL before appending it to the logical ID

Additional Information/Context

No response

CDK CLI Version

2.135.0

Framework Version

No response

Node.js Version

v20.11.1

OS

macOS 14.4.1

Language

TypeScript

Language Version

5.4.3

Other information

I'll open a PR to fix this shortly. I'll also check to make sure that invokeFunction and awsApiCall do not run into the same issue, and have tests to check it

@nmussy nmussy added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 3, 2024
@5nafu
Copy link

5nafu commented Apr 3, 2024

Thanks @nmussy for creating this and offereing to create a PR.

@tim-finnigan tim-finnigan self-assigned this Apr 3, 2024
@tim-finnigan tim-finnigan added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Apr 3, 2024
@tim-finnigan
Copy link

Thank you for opening this issue and creating the PR!

@tim-finnigan tim-finnigan added p2 effort/small Small work item – less than a day of effort and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Apr 3, 2024
@tim-finnigan tim-finnigan removed their assignment Apr 3, 2024
@pahud
Copy link
Contributor

pahud commented Apr 8, 2024

Yes I agree with you this should be sluggified.

new CfnOutput(node, 'AssertionResults', {
value: result,
}).overrideLogicalId(`AssertionResults${id}`);

Before we have a PR to address that, it would be easy to work it around without using . character in the ID.

@mergify mergify bot closed this as completed in #29705 Apr 8, 2024
mergify bot pushed a commit that referenced this issue Apr 8, 2024
### Issue # (if applicable)

Closes #29700

I've also opened #29701 to catch similar issues at synth time in the future

### Reason for this change

When running `httpApiCall('url').expect`, the `AssertionResults` output logical ID would be overridden with an invalid name, containing the URL slashes.

This issue was not noticed earlier because, as far as I can tell, assertions were only made with `Token`/`Ref` URLs, e.g. `apigw.DomainName`

### Description of changes

* Remove non-alphanumeric characters from the overridden `AssertionResults` output logical ID
* I've also added a bit of documentation to `ExpectedResult`, I noticed it was slightly lacking while creating the integration test

### Description of how you validated changes

I've added unit and integration tests. The integration tests include both tests with API Gateway, to cover unresolved URLs, and to https://httpbin.org/ to test this fix

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

github-actions bot commented Apr 8, 2024

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/integ-tests bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants