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

chore(custom-resources): running Python tests from Lerna #28860

Closed
wants to merge 3 commits into from

Conversation

sakurai-ryo
Copy link
Contributor

@sakurai-ryo sakurai-ryo commented Jan 25, 2024

Currently, the Python test code used in custom-resource-handlers is not included in the execution target of the lerna run test command and is not executed by CI.
This PR fixes the build process and the broken tests.

To run Python tests from Lerna, we need to run the shell script to run the Python tests in the test section of npm scripts.
It was originally executed using the post script of npm scripts.
https://github.com/aws/aws-cdk/pull/971/files#diff-a683107cab7bc3ac616d2db6e4e02c4f2aa677c56e45f0d8f822d82929a83d7dR41

The test section of npm scripts in the @aws-cdk/custom-resource-handlers package currently runs the jest command, which is used to run all tests in the package and specific tests.
I created a new shell script and branched out for these two use cases.

I am unfamiliar with Lerna or the CDK build process, so I would appreciate feedback from anyone who sees this PR.


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 bot added the p2 label Jan 25, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team January 25, 2024 10:43
@github-actions github-actions bot added the admired-contributor [Pilot] contributed between 13-24 PRs to the CDK label Jan 25, 2024
@@ -576,7 +576,7 @@ def test_fails_when_physical_id_not_present_in_update(self):
"DestinationBucketName": "<dest-bucket-name>",
}, expected_status="FAILED")

self.assertEqual(update_resp['Reason'], "invalid request: request type is 'Update' but 'PhysicalResourceId' is not defined")
self.assertEqual(update_resp['Reason'], "invalid request: request type is '{'Update'}' but 'PhysicalResourceId' is not defined")
Copy link
Contributor Author

@sakurai-ryo sakurai-ryo Jan 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed in this PR, but the test code did not.
#28057

@@ -62,10 +62,7 @@ def make_eventbridge_configuration():
return { EVENTBRIDGE_CONFIGURATION: {} }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR changed this file, but the test code did not.
#28132

@@ -86,337 +83,330 @@ def merge_notification_configurations(conf1: Dict, conf2: Dict):


class ManagedBucketTest(unittest.TestCase):
@patch("index.put_bucket_notification_configuration")
@patch('index.s3')
Copy link
Contributor Author

@sakurai-ryo sakurai-ryo Jan 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a function that executes the put_bucket_notification_configuration method of boto3, but it has been removed.
https://github.com/aws/aws-cdk/pull/28132/files#diff-00d1f29e73955f487795d4a9873bcff824e70d6268e7926e44b7defb31b4ecb1

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 25, 2024
Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, wow. This is a very very long term oversight on our part. We shouldn't have unit tests that aren't jest anymore, but taking this change is obviously much better than having tests that don't run. Thanks for catching this.

@TheRealAmazonKendra
Copy link
Contributor

@Mergifyio update

Copy link
Contributor

mergify bot commented Apr 17, 2024

update

❌ Mergify doesn't have permission to update

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/repo-metrics-monthly.yml without workflows permission

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 17, 2024
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 95140bd
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@TheRealAmazonKendra
Copy link
Contributor

Looks like some of the test updates caused a failure. Can you please take a look?

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

2 similar comments
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
admired-contributor [Pilot] contributed between 13-24 PRs to the CDK closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants