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(cli-lib-alpha): slightly more idiomatic use of mocks #32512

Closed
wants to merge 1 commit into from

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Dec 13, 2024

The original version of this code was failing for obscure reasons.


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

The original version of this code was failing for obscure reasons.
@rix0rrr rix0rrr requested a review from a team as a code owner December 13, 2024 13:24
@github-actions github-actions bot added the p2 label Dec 13, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team December 13, 2024 13:24
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Dec 13, 2024
import * as cli from 'aws-cdk/lib';
import * as core from 'aws-cdk-lib';
Copy link
Contributor

@mrgrain mrgrain Dec 13, 2024

Choose a reason for hiding this comment

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

This is was importing aws-cdk-lib/core on purpose. Curios why it needs changing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this doesn't seem to work in the new monorepo, I assume because aws-cdk-lib has been bundled.

Why must it import the submodule directly?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because it is best practice to import submodules and if that's not working anymore in the new monorepo that is extremely concerning to me. Especially if we don't understand why it is not working anymore.

Copy link
Contributor

@mrgrain mrgrain left a comment

Choose a reason for hiding this comment

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

What's the obscure reason? Someone in the future will need to know this.

Copy link

codecov bot commented Dec 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.66%. Comparing base (934c9dc) to head (fab881b).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #32512   +/-   ##
=======================================
  Coverage   78.66%   78.66%           
=======================================
  Files         107      107           
  Lines        7161     7161           
  Branches     1320     1320           
=======================================
  Hits         5633     5633           
  Misses       1343     1343           
  Partials      185      185           
Flag Coverage Δ
suite.unit 78.66% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
packages/aws-cdk 78.66% <ø> (ø)

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@rix0rrr rix0rrr closed this Dec 20, 2024
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
contribution/core This is a PR that came from AWS. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants