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

Tests ARM, test case for ExtensionResource #701

Closed
wants to merge 27 commits into from

Conversation

v-hongli1
Copy link
Contributor

@v-hongli1 v-hongli1 commented Aug 15, 2024

Cadl Ranch Contribution Checklist:

  • I have written a scenario spec
  • I have meaningful @scenario names. Someone can look at the list of scenarios and understand what I'm covering.
  • I have written a mock API
  • I have used @scenarioDocs for extra scenario description and to tell people how to pass my mock api check.

v-hongli1 added a commit to v-hongli1/cadl-ranch that referenced this pull request Aug 15, 2024
@@ -907,6 +907,156 @@ Expected response body:
}
```

### Azure_ResourceManager_Models_Resources_ExtensionsResources_createOrUpdate

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: do we need ExtensionsResources, or is just Extensions ok bc it's in the Resources subspace

Copy link
Contributor

@XiaofeiCao XiaofeiCao Aug 29, 2024

Choose a reason for hiding this comment

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

Extension is good.

Glad you brought it up. In fact, I revisited our existing ARM cases:
Screenshot 2024-08-29 at 12 25 21

Some of them contain duplicate like this, and some not appropriate, e.g. ManagedIdentityTrackedResources..
I've made a refactor PR to decouple SDK generated interface name with the scenarioName(it shouldn't affect existing generated SDKs):
#714

I had to use clientName to achieve that, as @scenario("xx") on the interface require extra mock tests for it.. Let me know if there are better solutions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I'm confused why you would need to use clientName. Can you clarify what is failing / needing extra tests because that shouldn't be happening. Here's an example of a renaming pr, if that helps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by commit 8ca02c5

Copy link
Contributor

@XiaofeiCao XiaofeiCao Aug 30, 2024

Choose a reason for hiding this comment

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

Thanks, it's mainly because that I don't want to break existing generated SDKs from current cadl-ranch. We already have several languages wrote test for generated code.
Wonder if there's better way to achieve that rather than @clientName..

Copy link
Contributor

Choose a reason for hiding this comment

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

In cases like these, we should go ahead and break. To do so, make sure everyone is aware of this issue, and that the PR has a minor version bump. Writing a mitigation guide (can just be a few lines: change x import to x import) is also very welcome.

Copy link
Contributor

Choose a reason for hiding this comment

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

@iscai-msft May I know the concern of @clientName? There's one more potential use case: #714 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

somehow I totally missed this comment, my bad. The concern is that we're using @clientName to get around breaking cadl-ranch, and we're ok breaking cadl-ranch as long as everyone is aware. It adds technical debt and also makes the tests not as atomic: we aren't explicitly testing @clientName here so we shouldn't be using it

@XiaofeiCao XiaofeiCao mentioned this pull request Aug 29, 2024
4 tasks
@v-hongli1 v-hongli1 closed this Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants