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.

Sign up for GitHub

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

Jump to bottom

integ-tests: invalid AssertionResults logical ID #29700

Closed
nmussy opened this issue Apr 3, 2024 · 5 comments · Fixed by #29705 or rwlxxvii/containers#124 · May be fixed by NOUIY/aws-solutions-constructs#98, NOUIY/aws-solutions-constructs#99 or NOUIY/aws-solutions-constructs#101
Closed

integ-tests: invalid AssertionResults logical ID #29700

nmussy opened this issue Apr 3, 2024 · 5 comments · Fixed by #29705 or rwlxxvii/containers#124 · May be fixed by NOUIY/aws-solutions-constructs#98, NOUIY/aws-solutions-constructs#99 or NOUIY/aws-solutions-constructs#101
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 •
edited
Loading

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:

aws-cdk/packages/@aws-cdk/integ-tests-alpha/lib/assertions/http-call.ts

Lines 48 to 50 in 36fd79d

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
@nmussy nmussy mentioned this issue Apr 3, 2024
Closed
@5nafu
Copy link

5nafu commented Apr 3, 2024

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

@nmussy nmussy mentioned this issue Apr 3, 2024
Merged
1 task
@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
@github-actions github-actions bot mentioned this issue Apr 8, 2024
Closed
@pahud
Copy link
Contributor

pahud commented Apr 8, 2024

Yes I agree with you this should be sluggified.

aws-cdk/packages/@aws-cdk/integ-tests-alpha/lib/assertions/http-call.ts

Lines 48 to 50 in 36fd79d

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
@nmussy
fix(integ-tests): httpApiCall.expect with resolved URL (#29705)
49b4aa1 
### 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*
@github-actions GitHub Actions
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.

@nmussy nmussy mentioned this issue Apr 9, 2024
Merged
1 task
@github-actions github-actions bot mentioned this issue May 1, 2024
Closed
This was referenced May 23, 2024
Open
Open
@rwlxxvii rwlxxvii mentioned this issue May 27, 2024
Merged
@NOUIY NOUIY mentioned this issue May 30, 2024
Open
@NOUIY NOUIY mentioned this issue Jun 7, 2024
Open
@niravparikh05 niravparikh05 mentioned this issue Jun 13, 2024
Open
This was referenced Jun 14, 2024
Open
Open
@aws-cdk-automation
Copy link
Collaborator

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.

@aws aws locked as resolved and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
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
Milestone
No milestone
5 participants
@pahud @5nafu @nmussy @aws-cdk-automation @tim-finnigan